Make WordPress Core

Opened 3 weeks ago

Closed 9 days ago

Last modified 8 days ago

#64661 closed defect (bug) (fixed)

Code Editor: Linting runs twice at initialization

Reported by: westonruter's profile westonruter Owned by: westonruter's profile westonruter
Milestone: 7.0 Priority: normal
Severity: normal Version: 4.9
Component: General Keywords: has-patch
Focuses: javascript Cc:

Description

As reported by @jonsurrell:

I added some logging here (console.log( 'Parsing with: %o', getEspreeOptions( options ) )) to understand the options and I noticed something (note this is with https://github.com/westonruter/wordpress-develop/pull/5 so adds module config).

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:

Parsing with: {ecmaVersion: 'latest', sourceType: 'script', ecmaFeatures: {…}}
Parsing with: {ecmaVersion: 11, sourceType: 'module', ecmaFeatures: {…}}

And the lint is performed as expected. However, sometimes this is the order and the default linting is applied:

Parsing with: {ecmaVersion: 11, sourceType: 'module', ecmaFeatures: {…}}
Parsing with: {ecmaVersion: 'latest', sourceType: 'script', ecmaFeatures: {…}}

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.

This is a follow-up to the work to upgrade CodeMirror to the latest v5 (#48456) and to using Espree instead of Espree for JS linting (#64558).

Change History (10)

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


3 weeks ago
#1

This is a follow-up to:

To address:

In subsequent PRs:

  • [ ] Remove jQuery dependency
  • [ ] Add TypeScript to precommit task and GHA workflow.

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

## Gemini Summary

This branch introduces comprehensive TypeScript static analysis for JavaScript files using JSDoc, modernizes several Code Editor components, and resolves logical and build-process issues.

#### 1. TypeScript Static Analysis & Type Safety

  • Infrastructure: Introduced tsconfig.json configured for JS checking (checkJs: true) and strict mode (strict: true).
  • Global Definitions: Created typings/globals.d.ts to define global interfaces for wp, jQuery, _, Backbone, and HTMLHint.
  • Enhanced Type Definitions:
    • Defined precise typedefs in code-editor.js (CodeEditorInstance, LintingController, CombinedLintOptions, etc.) to replace generic Object and Function types.
    • Integrated CodeMirror addon types (lint, show-hint) using triple-slash directives.
    • Successfully eliminated almost all generic any usage in favor of specific intersection types and unions.
  • Strict Checking Enablement: Resolved numerous strict-mode issues, including null safety for optional callbacks and properties.

#### 2. Code Modernization & Refactoring

  • code-editor.js:
    • Refactored to use const and let variable declarations.
    • Replaced deprecated APIs: event.keyCode with event.key and substr() with slice().
    • Switched to native DOM APIs where appropriate (Element.classList, Node.contains).
    • Standardized multi-line object literals with trailing commas.
  • htmlhint-kses.js:
    • Refactored to modern ES syntax (const/let, arrow functions, template literals, and for...of loops).
    • Applied Prettier formatting for consistency.

#### 3. Build Process & Reorganization

  • File Relocation: Moved WordPress-maintained CodeMirror extensions (fakejshint.js, htmlhint-kses.js, javascript-lint.js) from vendor/ to lib/ to distinguish them from third-party vendor code.
  • Redundancy Cleanup: Deleted the redundant esprima.js source file; the build now correctly sources the latest version from the npm package.
  • Linter Optimization:
    • Fixed a race condition/double-linting bug during initialization by constructing complete options before editor creation.
    • Removed obsolete nested options.options for linting, following modern CodeMirror standards.
  • ESLint Alignment: Added a local .eslintrc.json in lib/codemirror/ to correctly signal ES module parsing for javascript-lint.js while maintaining compatibility for other files.

#### 4. Dependencies

  • Added @types/jquery, @types/codemirror, @types/underscore, @types/espree, and @types/htmlhint as devDependencies to support the static analysis effort.

All changes have been verified via tsc and jshint.

## Fixed PhpStorm Inspections

https://github.com/user-attachments/assets/d16c42b1-48d3-4c56-a75f-4470fb6dba1e

https://github.com/user-attachments/assets/ecf39df2-841f-4682-9f9a-f431eb529799

## Use of AI Tools

I used Gemini CLI for granular commits, each of which I reviewed.

#2 @westonruter
3 weeks ago

  • Owner set to westonruter
  • Status changed from new to accepted

This is closely related to #61175 which introduces PHPStan as the PR for this ticket introduces TypeScript, although currently constrained to apply to files related to the code editor. The pull request introduces TypeScript to help guide the refactor to how linting options are gathered so as to avoid the double-linting issue. Instead of initializing with the default options first and then supplying the WordPress options, now it gathers the WordPress options and supplies them into the code editor when it first initializes.

#3 @westonruter
3 weeks ago

  • Focuses javascript added

@westonruter commented on PR #10900:


3 weeks ago
#4

This was _not_ committed yet.

@westonruter commented on PR #10900:


2 weeks ago
#5

@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:


9 days ago
#6

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

@westonruter commented on PR #10900:


9 days ago
#7

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.

#8 @westonruter
9 days ago

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

In 61800:

Code Editor: Improve types and fix options handling to avoid double-linting at initialization.

  • Refactor how CodeMirror is initialized so that the full settings are provided up-front. This avoids the linting from being applied twice at initialization, the first time with an incorrect configuration.
  • Add initial TypeScript configuration for core with npm run typecheck:js.
  • Add comprehensive types for code editor files: code-editor.js, javascript-lint.js, and htmlhint-kses.js.
  • Move code editor scripts from src/js/_enqueues/vendor/codemirror/ to src/js/_enqueues/lib/codemirror/. The CodeMirror library is sourced from the npm package as of r61539.
  • Remove (deprecated) esprima.js from being committed to SVN since in r61539 it was switched to using the npm package as its source.
  • Move fakejshint.js to src/js/_enqueues/deprecated.

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

Follow up to r61611, r61539.

Props westonruter, jonsurrell, justlevine.
See #64662, #48456.
Fixes #64661.

@westonruter commented on PR #10900:


9 days ago
#9

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

#10 @westonruter
8 days ago

In 61830:

Build/Test Tools: Integrate TypeScript into the core development workflow.

This introduces a new GitHub Action workflow for JavaScript type checking, mirroring the implementation for PHPStan in #61175. It also adds a typecheck:js Grunt task and includes it in the precommit:js task list. Only files related to the code editor are initially checked with TypeScript, with the expectation that additional files will be added to the files list in tsconfig.json as a part of ongoing maintenance work, for example #64238 and #64226.

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

Follow up to r61699, r61800, r61539.

Props westonruter, jonsurrell.
See #61175, #64661, #48456.
Fixes #64662.

Note: See TracTickets for help on using tickets.