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

Fix vscode javascript syntax highlighting for ?? operator (nullish coalescing) #6205

Closed

Conversation

stevenhao
Copy link

@stevenhao stevenhao commented Oct 17, 2021

What kind of change does this PR introduce?

Fixes a bug

What is the current behavior?

See issue: #5587

What is the new behavior?

JS, TS, JSX, TSX files are syntax highlighted correctly even when the ?? operator is used.

What steps did you take to test this? This is required before we can merge, make sure to test the flow you've updated.

  1. Forked the original repro codesandbox to add tests for .js, .jsx, .ts, .tsx files
  2. Confirmed that the bug is present on production (https://codesandbox.io/s/confident-microservice-osg2x?file=/src/App.js:301-362) -- after a line containing the ?? operator, keywords like const are not correctly colored
    image
  3. Confirmed that the bug is not present locally (http://localhost:3000/s/confident-microservice-osg2x?file=/src/App.js) (covers the development build)
    image
  4. Confirmed that the bug is not present in the autodeployed preview for this PR https://pr6205.build.csb.dev/s/confident-microservice-osg2x?file=/src/App.js:301-362 (covers the production build as well)
    image

Notes

  1. Updated all tmLanguage files that contained the string logical.[jt]s that weren't test files, using add nullish coalescing support microsoft/TypeScript-TmLanguage#768 as a reference to update the patterns for keyword.operator.logical.* and ternary-expression.begin
  2. Followed readme of https://github.com/codesandbox/codesandbox-client/tree/master/standalone-packages/vscode-extensions/out/bundles to regenerate main.min.json (used an online json minifier since the referenced https://github.com/codesandbox/extension-bundle-utils repo is inaccessible to me) and then bump all version numbers (I used repo-wide find/replace)

Caveats

  1. I manually patched the tmLanguage files to support nullish coalescing, but I think a better fix would be to pull the latest version of https://github.com/microsoft/TypeScript-TmLanguage. It seems like there are scripts somewhere that do this, but I was unable to figure out how to get npm run update-grammars to work. It seems the readme in https://github.com/codesandbox/codesandbox-client/tree/master/standalone-packages/vscode-extensions/out/extensions/typescript-basics/syntaxes is out of date, and maybe it has to do with build being gitignored.
  2. I didn't closely inspect the diff to main.min.json. Would it make sense to un-minify this file & separate fields by newlines, for ease of review in the future?
  3. I had to skip precommit checks because yarn lint-staged crashed with OOM (this is probably due to the updates to the minified js files causing eslint to report tens of thousands of issues). If this is not a problem, would it make sense to exclude these from the precommit hook? (it seems reasonably straightforward to do this according to https://stackoverflow.com/questions/55457364/husky-lint-staged-is-it-possible-to-exclude-ignore-file).

Checklist

  • Documentation
  • Testing
  • Ready to be merged
  • Added myself to contributors table

@stevenhao stevenhao changed the title make it work Fix syntax highlighting for ?? operator (nullish coalescing) Oct 17, 2021
@stevenhao stevenhao changed the title Fix syntax highlighting for ?? operator (nullish coalescing) Fix javascript syntax highlighting for ?? operator (nullish coalescing) Oct 17, 2021
@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 17, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 4ad54ef:

Sandbox Source
Notifications Test Configuration
condescending-fermi-cr3mm PR

@lbogdan lbogdan temporarily deployed to pr6205 October 17, 2021 05:18 Inactive
@lbogdan
Copy link
Contributor

lbogdan commented Oct 17, 2021

Build for latest commit 4ad54ef is at https://pr6205.build.csb.dev/s/new.

@stevenhao stevenhao changed the title Fix javascript syntax highlighting for ?? operator (nullish coalescing) Fix vscode javascript syntax highlighting for ?? operator (nullish coalescing) Oct 17, 2021
@stevenhao
Copy link
Author

stevenhao commented Oct 17, 2021

It seems like my edits to main.min.json may actually be problematic, since the steps I took to regenerate it from scratch may have erased some other changes made in earlier commits. I'm not sure why this is the case, but it seems like a good reason to hold off on merging this PR until it's addressed.

@danilowoz do you happen to have any ideas as to what's going on?

Here's a python script I used to compare the two versions (from https://github.com/codesandbox/codesandbox-client/commits/1e79186645ef359c60f2b5958dffb06788ddddb1/standalone-packages/vscode-extensions/out/bundles/main.min.json):
diff_main_min_json_keys.py.txt

image

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

Successfully merging this pull request may close these issues.

3 participants