Make WordPress Core

Opened 7 weeks ago

Closed 5 weeks ago

Last modified 12 days ago

#64558 closed defect (bug) (fixed)

CodeMirror: JavaScript linting is incompatible with newer language features

Reported by: westonruter's profile westonruter Owned by: westonruter's profile westonruter
Milestone: 7.0 Priority: normal
Severity: normal Version: 4.9.3
Component: External Libraries Keywords: has-patch has-unit-tests
Focuses: javascript Cc:

Description (last modified by westonruter)

In #42850 (r42547), JSHint was replaced with Esprima due to the former having a GPL-incompatible license. However, the esprima package has not been updated since July 2018. Because it hasn't been updated in so long, it does not support "newer" syntax ECMAScript syntax (since 2020) like dynamic imports, optional chaining, and nullish coalescing:

async function getMod() {
    const mod = await import( 'https://example.com/module.js' );
    // ❌ ------------^
    return mod;
}

const baz = window?.foo?.bar?.baz;
// ❌ -------------^

const quux = baz ?? 'baz';
// ❌ ------------^

The Espree project (part of ESLint) is an actively-maintained substitute for Esprima.

As a follow-up to #48456 (r61539) which upgraded CodeMirror to the latest v5 and Esprima (from 4.0.0 to 4.0.1), we can now do a better upgrade by using Espree instead of Esprima.

Change History (16)

#1 @westonruter
7 weeks ago

  • Description modified (diff)

This ticket was mentioned in PR #10806 on WordPress/wordpress-develop by @westonruter.


7 weeks ago
#2

  • Keywords has-patch has-unit-tests added
  1. New Linter Integration:
    • Introduces src/js/_enqueues/vendor/codemirror/javascript-lint.js which uses espree (v9.6.1) for parsing and error reporting.
    • This replaces the previous dependency on the jshint and esprima scripts. The espree module is now loaded via a dynamic import on demand by the new javascript lint integration.
    • This custom linter is bundled into the CodeMirror build via tools/vendors/codemirror-entry.js.
  1. Script Modules:
    • Registers espree as a script module in src/wp-includes/script-modules.php.
    • Adds a workaround in the wp-codemirror registration to ensure espree is included in the importmap.
  1. Editor Settings:
    • Updates wp_get_code_editor_settings() in src/wp-includes/general-template.php to use ES11 defaults.
    • Synchronizes JSHint settings from .jshintrc, even though these are not supported by Espree.
  1. Deprecations:
    • Marks esprima and jshint script handles as deprecated in src/wp-includes/script-loader.php.
  1. Build Tools:
    • Updates Webpack configuration (tools/webpack/codemirror.config.js) to bundle espree as a module.
    • Updates codemirror-entry.js to use the new local javascript-lint.js.

Previously developed in https://github.com/westonruter/wordpress-develop/pull/4 as a sub-PR off of https://github.com/WordPress/wordpress-develop/pull/10778

Trac ticket: https://core.trac.wordpress.org/ticket/64558

#3 @westonruter
7 weeks ago

Related: #64562

Last edited 7 weeks ago by westonruter (previous) (diff)

#4 @westonruter
7 weeks ago

Currently blocked by: #61500

@jonsurrell commented on PR #10806:


5 weeks ago
#5

I noticed that the javascript-lint file isn't correctly watched when testing with npm run dev.

This isn't a blocker, but it's always nice when these tasks all work.

Grunt goes into watch mode, and changes to src/js/_enqueues/vendor/codemirror/javascript-lint.js triggers this:

>> File "src/js/_enqueues/vendor/codemirror/javascript-lint.js" changed.

Running "clean:dynamic" (clean) task
>> 0 paths cleaned.

Running "copy:dynamic-js" (copy) task
Copied 1 file

Running "uglify:dynamic" (uglify) task
>> 1 file created 3.59 kB → 759 B

Running "_watch" task

That copy/uglify seems redundant and maybe a codemirror watch task isn't set up?

@jonsurrell commented on PR #10806:


5 weeks ago
#6

I was testing this out and wanted to work with some script modules but that errors on import/export syntax. I created https://github.com/westonruter/wordpress-develop/pull/5 to add module support based on .mjs extension.

@westonruter commented on PR #10806:


5 weeks ago
#7

I noticed that the javascript-lint file isn't correctly watched when testing with npm run dev.

@sirreal I actually have trouble with npm run dev generally. I can't get watch to work reliably with any files. For example, I run it and then I modify src/js/_enqueues/lib/comment-reply.js, and yet in my terminal it just remains indefinitely at:

Running "_watch" task
Waiting...

This seems like a larger issue to be investigated.

#8 @westonruter
5 weeks ago

Note: espree v9.6.1 is used because it is already installed via @wordpress/scripts => eslint (v8.45.0) => espree (9.6+).

There is a newer version, v11.1.0, however. We should upgrade once we bump the minimum version of Node and/or @wordpress/scripts is updated to use a newer ESLint. Or maybe this is irrelevant and can go ahead and use v11.1.0 now. In any case, we can make that upgrade in a subsequent commit.

#9 @westonruter
5 weeks ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 61611:

Code Editor: Switch from Esprima to Espree for JavaScript linting in CodeMirror.

Esprima is no longer maintained, and it does not support the latest JavaScript features in ES11, as Espree does.

  • New Linter Integration: Introduces src/js/_enqueues/vendor/codemirror/javascript-lint.js using espree for parsing and error reporting, replacing the dependency on jshint and esprima scripts.
  • Script Modules: Registers espree as a script module and leverages the module_dependencies argument in wp_register_script() to ensure espree is available as a dynamic import.
  • Editor Settings: Updates wp_get_code_editor_settings() to use ES11 (ECMAScript 2020) defaults and synchronizes JSHint settings from .jshintrc for compatibility.
  • Editable Extensions: Adds .mjs to the list of editable file extensions for plugins and themes.
  • Deprecations: Marks esprima and jshint script handles as deprecated.
  • Build Tools: Updates Webpack configuration to bundle espree as a module and use the new local javascript-lint.js.

Developed in https://github.com/WordPress/wordpress-develop/pull/10806

Follow-up to [61587], [61544], [61539], [42547].

Props westonruter, jonsurrell.
See #64562, #61500, #48456, #42850.
Fixes #64558.

@westonruter commented on PR #10900:


4 weeks ago
#11

Note: The coding standards failure is due to an existing JSHint error with fakejshint.js which is now deprecated. The file was unchanged.

@westonruter commented on PR #10900:


3 weeks ago
#12

This was _not_ committed yet.

@westonruter commented on PR #10900:


3 weeks ago
#13

@sirreal:

I've left some feedback mostly around the TypeScript setup. If you'd like, I'm happy to work on adjustments and push changes.

Please do! Please amend as you see fit. I'm definitely not a TypeScript expert, so your expertise is much appreciated.

Frankly, I'd like to start using TypeScript syntax directly for these files that are already being compiled/bundled where we're adding a lot of type information.

That sounds good to me. I think in this PR that would mean we could turn src/js/_enqueues/lib/codemirror/javascript-lint.js and tools/vendors/codemirror-entry.js into TypeScript files. The other JS files, namely src/js/_enqueues/lib/codemirror/htmlhint-kses.js and src/js/_enqueues/wp/code-editor.js I think should remain as JS since they should be able to be run directly in the browser. (For example, I symlink them to src/wp-includes to avoid having to run a build when I make changes, although I'm willing to concede if this is just me.)

Whether a file is TypeScript or not could also serve as the condition for whether to check types.

There is a lot of type information added to code-editor.js though, so we should be sure to add that as an allow-list of JS files that we also start type-checking. I really would like to start incrementally adding more and more of the core JS files to beying type-checked as well.

@jonsurrell commented on PR #10900:


13 days ago
#14

The typcheck:js script doesn't run on CI, does it? That would be a good follow-up.

@westonruter commented on PR #10900:


13 days ago
#15

Thank you!

The typcheck:js script doesn't run on CI, does it? That would be a good follow-up.

Yes, that is what Core-64662 for.

@westonruter commented on PR #10900:


12 days ago
#16

The typcheck:js script doesn't run on CI, does it? That would be a good follow-up.

Ready for review: https://github.com/WordPress/wordpress-develop/pull/11131

Note: See TracTickets for help on using tickets.