WordPress.org

Make WordPress Core

Opened 2 months ago

Last modified 3 weeks ago

#41870 assigned task (blessed)

Code Editor: Add grunt task for building new CodeMirror bundles from external dependencies

Reported by: westonruter Owned by: adamsilverstein
Milestone: 5.0 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch early needs-testing
Focuses: javascript Cc:

Description (last modified by westonruter)

After [41376] for #12423, it would be ideal if the CodeMirror bundling logic (via browserify) could be added as part of core. In this way, the external dependencies (CodeMirror and the linter linraries) can be managed by NPM. For now, just the minified bundles are being included.

The logic used in the Better Code Editing plugin for creating the bundles can be seen here: https://github.com/WordPress/better-code-editing/pull/92/files

Depends on #40894 which replaces Browserify with Webpack in core.

See also #42424, where we need to fix line-endings for CSSLint.

Additionally, the linters are currently being included as separate assets. They are not being minified, other than they are already minified as distributed. These could either benefit from additional uglification or bundling with codemirror if they are not going to be enqueued separately. The same goes for the htmlhint-kses script.

Attachments (11)

41870.diff (2.5 MB) - added by adamsilverstein 5 weeks ago.
41870.2.diff (3.1 MB) - added by adamsilverstein 5 weeks ago.
41870.3.diff (3.1 MB) - added by adamsilverstein 5 weeks ago.
41870.4.diff (3.1 MB) - added by adamsilverstein 5 weeks ago.
41870.5.diff (3.5 MB) - added by adamsilverstein 5 weeks ago.
41870.6.diff (3.5 MB) - added by adamsilverstein 5 weeks ago.
41870.7.diff (4.2 MB) - added by adamsilverstein 5 weeks ago.
41870.8.diff (5.2 MB) - added by adamsilverstein 5 weeks ago.
41870.9.diff (4.6 MB) - added by adamsilverstein 5 weeks ago.
41870.10.diff (3.1 MB) - added by adamsilverstein 5 weeks ago.
41870.11.diff (7.2 MB) - added by adamsilverstein 5 weeks ago.

Change History (52)

#1 @westonruter
2 months ago

  • Description modified (diff)

#2 @westonruter
2 months ago

  • Description modified (diff)

#3 @westonruter
2 months ago

  • Priority changed from normal to high

Bumping priority to high for visibility and alignment with 4.9 goals, and given proximity to beta 1 deadline.

#4 @westonruter
2 months ago

  • Type changed from enhancement to task (blessed)

#5 @westonruter
2 months ago

CodeMirror 5.30.0 was just released so this would be a good update to test the integration of a build process.

#6 @westonruter
2 months ago

  • Focuses javascript added

#7 @westonruter
2 months ago

  • Component changed from External Libraries to Build/Test Tools

#8 @westonruter
8 weeks ago

This also needs to be done for MediaElement.js

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


5 weeks ago

#10 @westonruter
5 weeks ago

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

#11 @adamsilverstein
5 weeks ago

  • Keywords has-patch dev-feedback added; needs-patch removed

In 41870.diff:

  • Use webpack to build codemirror.js from manifest based on https://github.com/WordPress/better-code-editing/pull/92/files
  • Switch script loader to use the unminified script when SCRIPT_DEBUG is true
  • Include codemirror 5.30 from npm
  • Removed the minified file, including the unminified file - the minified file should be created in the release build (i'll test that)

@westonruter I wasn't sure about the linters, you want to bundle those as well so its one single bundle?

Also, thinking I should remove the watch on these files, any reason not to?

#13 @adamsilverstein
5 weeks ago

41870.3.diff add codemirror to uglify for build

#14 follow-up: @adamsilverstein
5 weeks ago

This also needs to be done for MediaElement.js

Is this built from dependencies as well? any more details?

#15 in reply to: ↑ 14 @westonruter
5 weeks ago

Replying to adamsilverstein:

This also needs to be done for MediaElement.js

Is this built from dependencies as well? any more details?

@adamsilverstein Let's re-visit ME.js later. We should get details from @rafa8626 on how the current scripts are getting built at that time.

#16 @rafa8626
5 weeks ago

@westonruter @adamsilverstein The ME.js file that it's been used currently is already minified and uglified. The uncompressed version is bundled via Browserify/Grunt. None of the source packages are included in the mediaelement folder. Let me know if you have any other questions/comments

#17 @adamsilverstein
5 weeks ago

41870.4.diff remove the watch for dev build

#18 @westonruter
5 weeks ago

  • Keywords dev-feedback removed

As we discussed on Slack, the next steps are:

#19 @adamsilverstein
5 weeks ago

I started working on this and got part way there https://github.com/WordPress/wordpress-develop/compare/master...adamsilverstein:ticket-41870?expand=1. One issue I need to resolve is adding htmlhint-kses.js after htmlhint.js so that it can extend init for HTMLHint.

#20 @adamsilverstein
5 weeks ago

41870.5.diff :

  • bundle the linters
  • expose the linters on the wp namespace
  • remove tests and other script handling code
Last edited 5 weeks ago by adamsilverstein (previous) (diff)

#21 @westonruter
5 weeks ago

@adamsilverstein It looks great!

The only thing I see that needs to be changed is the HTMLHint dependency needs to refer to the xwp/HTMLHint fork on GitHub (as seen in the plugin's package.json). The reason is that there is a bug in HTMLHint for which I've opened a PR but it has not been merged. Without this, the globals defined in the JSHint configuration are not properly passed, resulting in linting errors on each line when editing an HTML file that contains (yes, these will result in runtime errors too 😁):

<script>
wp();
Backbone();
_();
</script>

While if you edit a JS file these will not cause “not defined” warnings because they are declared in the config. But HTMLHint is failing to pass along these globals.

Unfortunately HTMLHint hasn't been updated since February so I'm not sure when we'd be able to switch the dependency back to the origin.

#22 @westonruter
5 weeks ago

@adamsilverstein one more thing. The travis:js build just failed in jsvalidate:build because of:

Line 5: Unexpected reserved word

It looks due to the import statement, and jsvalidate not expecting ES5. So maybe codemirror.manifest.js just needs to either be excluded from the build or just excluded from jsvalidate.

#23 @adamsilverstein
5 weeks ago

In 41870.6.diff: Skip codemirror manifest when copying to build.

I'll work on the other requested change soon.

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


5 weeks ago

#25 @adamsilverstein
5 weeks ago

@westonruter I wasn't able to reproduce the issue you mentioned, however I switched over to the fork anyway - in 41870.8.diff with some more cleanup for the changeset, can you give this a test?

I'm not certain about the placement of the manifest file, perhaps this along with the media manifest files belong in a common location? Also: it would be nice to standardize/automate the placement of packages on the wp namespace instead of enumerating them manually - maybe something similar to what is being done in Gutenberg.

#26 @adamsilverstein
5 weeks ago

41870.9.diff removes some unrelated changes from my diff against master.

#27 @westonruter
5 weeks ago

  • Keywords needs-patch added; has-patch removed

I can confirm that 41870.9.diff does fixes the issue with HTMLHint passing the JSHint config properly.

To recap where we're at after Slack DM:

  • The current webpack build causes very strange behavior in Customizer where all panels and sections get hidden with display:none except for the Widgets panel.
  • It would be ideal if JSHint, HTMLHint, CSSLint, and jsonlint didn't have to be added to the global scope in addition to wp. I recall being able to avoid this by using browserify-shim since the JSHint-lint addon in CodeMirror is not updated to work with modules. See https://github.com/codemirror/CodeMirror/pull/4962#issuecomment-329017493

#28 @adamsilverstein
5 weeks ago

Lets try leaving the linters externalized, we can work on adding separate builds for them as well so they get imported from the external npm libraries.

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


5 weeks ago

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


5 weeks ago

#31 @adamsilverstein
5 weeks ago

41870.10.diff externalizes the linters and restores their enqueue logic in src/wp-includes/general-template.php. This leaves codemirror as being built, and the customizer working... going to look at building the linters as well via webpack directly.

#32 @adamsilverstein
5 weeks ago

  • Keywords has-patch added; needs-patch removed

In 41870.10.diff I added a build process for the linters. Each linter gets a manifest, and webpack loops thru them, imports and exports a build file, automatically adding them to the wp namespace.

Noting that i'm simply duplicating code here between the dev and prod webpack configs - might make sense to combine them for now with a config toggle for the only differences between the two? We can also split them later if the approaches become very different.

What do you think of this approach @westonruter?

#33 @westonruter
5 weeks ago

@adamsilverstein I'm noticing that 41870.11.diff is over twice as large as the previous patch. Is 7MB patch expected?

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.


4 weeks ago

#35 @westonruter
4 weeks ago

(I agree we should keep the linters excluded from the bundle now that I see that JSHint is larger than the CodeMirror bundle itself!)

#36 @adamsilverstein
4 weeks ago

@westonruter despite the large size, i think the patch is correct - it has grown so large because it includes built replacements for the linters. Probably worth holding off for 5.0 early so this can get some wider testing and since we are so far into beta now, I trust you to make that call.

#37 @westonruter
4 weeks ago

  • Keywords early needs-testing added
  • Milestone changed from 4.9 to 5.0

Yeah, probably a good call.

#38 @westonruter
4 weeks ago

  • Priority changed from high to normal

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


4 weeks ago

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.


3 weeks ago

#41 @westonruter
3 weeks ago

  • Description modified (diff)

Note: The line endings of CSSLint need to be fixed as part of this. See #42424.

Note: See TracTickets for help on using tickets.