-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Switch from Esprima to Espree for JavaScript linting in CodeMirror. #10806
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
Closed
westonruter
wants to merge
24
commits into
WordPress:trunk
from
westonruter:replace-esprima-with-espree
Closed
Changes from all commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
a722b50
Switch from Esprima to Espree for JavaScript linting in CodeMirror.
westonruter bd2376c
Simplify javascript-lint module definition and ensure proper types
westonruter 9d5d79a
Merge branch 'trunk' of https://github.com/WordPress/wordpress-develo…
westonruter daa5e2e
Squash sirreal:scripts/allow-script-module-dependency (https://github…
westonruter fe846ac
CodeMirror: Use native dynamic import for Espree to allow Import Map …
westonruter 34f5e4c
Leverage the module_dependencies arg to add espree to importmap
westonruter 57cd3b0
Fix minification of espree after debugging
westonruter 99a6994
CodeMirror: Unwrap javascript-lint.js IIFE and modernize with const.
westonruter 0f8878d
CodeMirror: Add since 7.0.0 JSDoc tags to javascript-lint.js.
westonruter 89c8db8
Change method for suppressing JSHint warning for console.warn() in ja…
westonruter 0edaefa
Squash sirreal:scripts/allow-script-module-dependency (https://github…
westonruter 7a8d756
Merge branch 'trunk' of https://github.com/WordPress/wordpress-develo…
westonruter 9e3038d
Convert vendor source files to ESM imports.
westonruter 87cd175
Clarify comment for JSHint rules
westonruter 8ce6f62
Merge branch 'trunk' into replace-esprima-with-espree
westonruter ca2ff24
Merge branch 'trunk' of https://github.com/WordPress/wordpress-develo…
westonruter 2661c91
Simplify Webpack configuration for Espree.
westonruter 64f01f2
Merge branch 'trunk' into replace-esprima-with-espree
westonruter f2ee120
Add MJS and sourceType: module support
sirreal 5b68233
Use bool directly
sirreal 09a7bab
Add to editable theme files as well.
sirreal fbf63ef
Merge pull request #5 from sirreal/javascript-support-mjs
westonruter 2dd7d20
Merge branch 'trunk' of https://github.com/WordPress/wordpress-develo…
westonruter 083a8a8
Update tests to account for new module arg
westonruter File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,121 @@ | ||
| /** | ||
| * CodeMirror JavaScript linter. | ||
| * | ||
| * @since 7.0.0 | ||
| */ | ||
|
|
||
| import CodeMirror from 'codemirror'; | ||
|
|
||
| /** | ||
| * CodeMirror Lint Error. | ||
| * | ||
| * @see https://codemirror.net/5/doc/manual.html#addon_lint | ||
| * | ||
| * @typedef {Object} CodeMirrorLintError | ||
| * @property {string} message - Error message. | ||
| * @property {'error'} severity - Severity. | ||
| * @property {CodeMirror.Position} from - From position. | ||
| * @property {CodeMirror.Position} to - To position. | ||
| */ | ||
|
|
||
| /** | ||
| * JSHint options supported by Espree. | ||
| * | ||
| * @see https://jshint.com/docs/options/ | ||
| * @see https://www.npmjs.com/package/espree#options | ||
| * | ||
| * @typedef {Object} SupportedJSHintOptions | ||
| * @property {number} [esversion] - "This option is used to specify the ECMAScript version to which the code must adhere." | ||
| * @property {boolean} [es5] - "This option enables syntax first defined in the ECMAScript 5.1 specification. This includes allowing reserved keywords as object properties." | ||
| * @property {boolean} [es3] - "This option tells JSHint that your code needs to adhere to ECMAScript 3 specification. Use this option if you need your program to be executable in older browsers—such as Internet Explorer 6/7/8/9—and other legacy JavaScript environments." | ||
| * @property {boolean} [module] - "This option informs JSHint that the input code describes an ECMAScript 6 module. All module code is interpreted as strict mode code." | ||
| * @property {'implied'} [strict] - "This option requires the code to run in ECMAScript 5's strict mode." | ||
| */ | ||
|
|
||
| /** | ||
| * Validates JavaScript. | ||
| * | ||
| * @since 7.0.0 | ||
| * | ||
| * @param {string} text - Source. | ||
| * @param {SupportedJSHintOptions} options - Linting options. | ||
| * @returns {Promise<CodeMirrorLintError[]>} | ||
| */ | ||
| async function validator( text, options ) { | ||
| const errors = /** @type {CodeMirrorLintError[]} */ []; | ||
| try { | ||
| const espree = await import( /* webpackIgnore: true */ 'espree' ); | ||
| espree.parse( text, { | ||
| ...getEspreeOptions( options ), | ||
| loc: true, | ||
| } ); | ||
| } catch ( error ) { | ||
| if ( | ||
| // This is an `EnhancedSyntaxError` in Espree: <https://github.com/brettz9/espree/blob/3c1120280b24f4a5e4c3125305b072fa0dfca22b/packages/espree/lib/espree.js#L48-L54>. | ||
| error instanceof SyntaxError && | ||
| typeof error.lineNumber === 'number' && | ||
| typeof error.column === 'number' | ||
| ) { | ||
| const line = error.lineNumber - 1; | ||
| errors.push( { | ||
| message: error.message, | ||
| severity: 'error', | ||
| from: CodeMirror.Pos( line, error.column - 1 ), | ||
| to: CodeMirror.Pos( line, error.column ), | ||
| } ); | ||
| } else { | ||
| console.warn( '[CodeMirror] Unable to lint JavaScript:', error ); // jshint ignore:line | ||
| } | ||
| } | ||
|
|
||
| return errors; | ||
| } | ||
|
|
||
| CodeMirror.registerHelper( 'lint', 'javascript', validator ); | ||
|
|
||
| /** | ||
| * Gets the options for Espree from the supported JSHint options. | ||
| * | ||
| * @since 7.0.0 | ||
| * | ||
| * @param {SupportedJSHintOptions} options - Linting options for JSHint. | ||
| * @return {{ | ||
| * ecmaVersion?: number|'latest', | ||
| * ecmaFeatures?: { | ||
| * impliedStrict?: true | ||
| * } | ||
| * }} | ||
| */ | ||
| function getEspreeOptions( options ) { | ||
| const ecmaFeatures = {}; | ||
| if ( options.strict === 'implied' ) { | ||
| ecmaFeatures.impliedStrict = true; | ||
| } | ||
|
|
||
| return { | ||
| ecmaVersion: getEcmaVersion( options ), | ||
| sourceType: options.module ? 'module' : 'script', | ||
| ecmaFeatures, | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Gets the ECMAScript version. | ||
| * | ||
| * @since 7.0.0 | ||
| * | ||
| * @param {SupportedJSHintOptions} options - Options. | ||
| * @return {number|'latest'} ECMAScript version. | ||
| */ | ||
| function getEcmaVersion( options ) { | ||
| if ( typeof options.esversion === 'number' ) { | ||
| return options.esversion; | ||
| } | ||
| if ( options.es5 ) { | ||
| return 5; | ||
| } | ||
| if ( options.es3 ) { | ||
| return 3; | ||
| } | ||
| return 'latest'; | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some logging here (
console.log( 'Parsing with: %o', getEspreeOptions( options ) )) to understand the options and I noticed something (note this is with westonruter#5 so addsmoduleconfig).The plugin linting seems to be triggered twice, once with what appear to be defaults and again with the expected config. This creates a race, where sometimes on load this is printed:
And the lint is performed as expected. However, sometimes this is the order and the default linting is applied:
This does seem to be happening before this PR, but it seems very consistent. I'm always seeing it run lint with the desired options second.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm seeing that too.
I tried turning off minification and I captured the stack traces for the first and second invocation:
First
Second
Diff:
The second call is happening when the
lintoption gets updated here:wordpress-develop/src/js/_enqueues/wp/code-editor.js
Line 152 in 163bc04
The two calls are happening here:
wordpress-develop/src/js/_enqueues/wp/code-editor.js
Lines 298 to 300 in 163bc04
So it makes sense that it would be called twice, once with the default config and again with the custom config. The initial implementation is clearly not ideal, as this
configureLinting()function should be refactored to construct thelintoption earlier to be passed in initially in the call tofromTextArea().I'll include this work in the follow-up PR to improve the typing for
code-editor.js.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue is fixed in #10900!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After implementing TypeScript types for the JS files, I then prompted Gemini to resolve the issue based on what I thought needed to be done, as well as providing our conversation as context. Quite pleased 😄