Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for WGSL #3455

Merged
merged 31 commits into from
May 18, 2022
Merged

Conversation

Dr4gonthree
Copy link
Contributor

@Dr4gonthree Dr4gonthree commented May 15, 2022

This adds support for WGSL, the WebGPU Shading Language.

@github-actions
Copy link

github-actions bot commented May 15, 2022

JS File Size Changes (gzipped)

A total of 2 files have changed, with a combined diff of +1.49 KB (+55.6%).

file master pull size diff % diff
components/prism-wgsl.min.js 0 Bytes 1.48 KB +1.48 KB +100.0%
plugins/show-language/prism-show-language.min.js 2.67 KB 2.68 KB +8 B +0.3%

Generated by 🚫 dangerJS against 8bb57f7

Copy link
Member

@RunDevelopment RunDevelopment left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR @Dr4gonthree!

The implementation itself looks really good.

However, Prism still supports some older browsers, so we have to remove some newer regexes features like unicode mode and lookbehinds. This is why the lint CI fails.

I also noticed that you often split tokens into multiple patterns. While this is fine, we typically prefer to merge those patterns into one regex (if the resulting regex isn't too complex). We do this because it results in smaller file sizes and because our regex tooling works on a per-regex basis.

Also, some keywords aren't tested.

components/prism-wgsl.js Outdated Show resolved Hide resolved
components/prism-wgsl.js Outdated Show resolved Hide resolved
components/prism-wgsl.js Outdated Show resolved Hide resolved
components/prism-wgsl.js Outdated Show resolved Hide resolved
components/prism-wgsl.js Outdated Show resolved Hide resolved
components/prism-wgsl.js Outdated Show resolved Hide resolved
components/prism-wgsl.js Outdated Show resolved Hide resolved
components/prism-wgsl.js Outdated Show resolved Hide resolved
components/prism-wgsl.js Outdated Show resolved Hide resolved
components/prism-wgsl.js Outdated Show resolved Hide resolved
@Dr4gonthree
Copy link
Contributor Author

Thanks for all the feedback and suggestions @RunDevelopment.
I hope the new changes fix all the issues you pointed out.
All the ci jobs seem to have succeeded.

Copy link
Member

@RunDevelopment RunDevelopment left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found some minor things.

tests/languages/wgsl/attributes_feature.test Outdated Show resolved Hide resolved
components/prism-wgsl.js Outdated Show resolved Hide resolved
components/prism-wgsl.js Outdated Show resolved Hide resolved
components/prism-wgsl.js Outdated Show resolved Hide resolved
tests/languages/wgsl/operator_feature.test Outdated Show resolved Hide resolved
components/prism-wgsl.js Outdated Show resolved Hide resolved
@RunDevelopment
Copy link
Member

All the ci jobs seem to have succeeded.

CI jobs can be abused to send spam, mine cryptocurrency, or other stuff. To prevent this, not all CI jobs trigger for first-time contributors automatically. A maintainer (e.g. me) has to manually approve that the CI jobs are allowed to run for each commit you make.

Unfortunately, this means that the CI may fail long after you make a commit. However, you can run tests (npm run test), lints (npm run lint), and coverage (npm run regex-coverage) locally. This is most of what the CI also does.

@RunDevelopment
Copy link
Member

Please run npm run lint:fix to fix the linting error.

@RunDevelopment RunDevelopment merged commit 4c87d41 into PrismJS:master May 18, 2022
@RunDevelopment
Copy link
Member

Thank you for contributing @Dr4gonthree!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants