Make WordPress Core

Opened 8 months ago

Closed 7 months ago

Last modified 4 days ago

#58645 closed defect (bug) (fixed)

Remove outdated jsvalidate task from Gruntfile

Reported by: kadamwhite's profile kadamwhite Owned by: kadamwhite's profile kadamwhite
Milestone: 6.3 Priority: normal
Severity: normal Version: 6.3
Component: Build/Test Tools Keywords: has-patch commit
Focuses: javascript Cc:

Description

We should remove the outdated grunt-jsvalidate step in our JS build process to eliminate distracting false-positive errors and complications when introducing new valid JS into WordPress core.

While working on improvements to the Emoji Loader in https://github.com/WordPress/wordpress-develop/pull/4562 , @westonruter identified that the grunt-jsvalidate task used in Core's Gruntfile build process was reporting false-positive JS parsing issues.

This validation task was added in #26615 to pass generated JS assets through Esprima in order to verify that the minification step did not introduce parsing errors in the JS engines of the time (notably Internet Explorer, as you might expect).

grunt-jsvalidate has not been updated in a decade, and uses an outdated version of Esprima that does not understand well-established modern JavaScript syntax. The supported browser landscape has changed, and the version of Uglify JS we use has been continually improved in the ten years since we added this validator (our current version of Uglify, consumed through grunt-contrib-uglify, is less than a year old).

In Slack, @joemcgill proposed removing the validation step,

With many files now being written in ES6 or included from built files, should we consider removing this step entirely? Looking at Trac search results, it looks like many times jsvalidate is not catching legitimate errors, but instead needs to be worked around when updating libraries or bumping our own npm packages.

See @youknowriad's comment from earlier today as an example.

Alternately, if this is still needed in very specific cases, perhaps we should start to narrow the scope of which files are validated during builds so we can deprecate and remove this task over time?

I (@kadamwhite) support removing grunt-jsvalidate entirely, on the grounds that supported browser parser reliability is much higher than it was ten years ago, and that the complexity of updating this build step to a modern solution is not worth the value it provides the project. @youknowriad also supports removal, "I see very little value in that validation step."

@desrosj deferred to @jorbin, who notes,

I'm of the opinion that the usefulness of jsvalidate is no longer there and we should remove it. The background is as suspected, there were some errors in minification, usually in browsers we weren't regularly testing, and this [grunt-jsvalidate] was seen as a way to reduce those.

While it's certainly possible for there to still be an issue, I think that minifiers are ALOT better now and the JS engines WordPress target are much more modern and similar than 10 years ago.

Ticket adapted from slack thread https://wordpress.slack.com/archives/C5UNMSU4R/p1686977423731239

Change History (12)

This ticket was mentioned in Slack in #core-js by kadamwhite. View the logs.


8 months ago

#2 @joemcgill
8 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 6.3

I'm adding this to the 6.3 release milestone for consideration. I'd like to see us try to make this change early in the beta cycle so we have several releases to test this prior to RC1.

This ticket was mentioned in PR #4724 on WordPress/wordpress-develop by kadamwhite.


8 months ago
#3

  • Keywords has-patch added; needs-patch removed

#4 @westonruter
8 months ago

Note that this is no longer required for #58472 since I've updated the PR to make it ES3-compatible. In the future it would be great to be able to use async/await.

Version 0, edited 8 months ago by westonruter (next)

#6 @joemcgill
8 months ago

  • Keywords commit added

@kadamwhite I think this is ready to go, right?

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


8 months ago

This ticket was mentioned in Slack in #core by chaion07. View the logs.


8 months ago

#9 @swissspidy
8 months ago

  • Component changed from General to Build/Test Tools

#10 @mukesh27
8 months ago

  • Owner set to kadamwhite
  • Status changed from new to assigned

This ticket was discussed during bug scrub.

@kadamwhite Could you take care of this as PR got enough approval? Assign to you for commit.

#11 @kadamwhite
7 months ago

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

In 56247:

Build Tools: Remove outmoded jsvalidate task from Gruntfile.

grunt-jsvalidate was added to catch situations where minification introduced parsing errors in Internet Explorer.
IE is no longer supported by WordPress, and the grunt task itself has not been maintained for a decade.
Removing this task simplifies the build and avoids false-positive warnings where the outdated tool flags modern JS syntax incorrectly.

Props westonruter, joemcgill, jorbin, youknowriad, desrosj, swissspidy.
Fixes #58645. See #26615.

Note: See TracTickets for help on using tickets.