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

[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.

[westonruter]

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

[westonruter]

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

[westonruter]

This also needs to be done for MediaElement.js

[adamsilverstein]

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?

[adamsilverstein]

corrected diff in 41870.2.diff​

[adamsilverstein]

41870.3.diff​ add codemirror to uglify for build

[adamsilverstein]

This also needs to be done for MediaElement.js

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

[westonruter]

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.

[rafa8626]

@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

[adamsilverstein]

41870.4.diff​ remove the watch for dev build

[westonruter]

As we discussed on Slack, the next steps are:

• Bundle the linters with wp-codemirror, but expose them under the wp global namespace for plugins that want to add new rules.
• Remove this entire block of code in wp_enqueue_code_editor() (with updating some unit tests that check for specific linter scripts to be enqueued): ​https://github.com/WordPress/wordpress-develop/blob/fef4f1303a0c9ac68afcb29f763a3aee531563e5/src/wp-includes/general-template.php#L3451-L3491
• Remove the separate linter sources from SVN and their entries in script-loader.php

[adamsilverstein]

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.

[adamsilverstein]

41870.5.diff​ :

• bundle the linters
• expose the linters on the wp namespace
• remove tests and other script handling code

[westonruter]

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

[westonruter]

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

[adamsilverstein]

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

I'll work on the other requested change soon.

[adamsilverstein]

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

[adamsilverstein]

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

[westonruter]

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

[adamsilverstein]

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.

[adamsilverstein]

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.

[adamsilverstein]

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?

[westonruter]

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

[westonruter]

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

[adamsilverstein]

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

[westonruter]

Yeah, probably a good call.

[westonruter]

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

[adamsilverstein]

41870.12.diff​ refresh against trunk.

[WraithKenny]

I was playing around with the CodeMirror instance, attempting to add a CoffeeScript mode, and noticed that CodeMirror is acting as if it's in CommonJS environment instead of Browser environment.

The static bundler (whether Webpack, Rollup, or Browserify) causes this issue, and the issue prevents being able to add Modes, even via the included loadmode.js plugin.

It might be better to just use grunt/gulp concatenate and minify (wrapped in an IIFE to place it in wp.CodeMirror).

​https://github.com/codemirror/CodeMirror/issues/4838

(P.S. I haven't tested Webpack, Rollup, or Browserify, but Webpack was mentioned in a bug report on the CodeMirror Repo, and the current version is via Browswerify. I assume Rollup is the same.)

[netweb]

I've not looked at the patches in this ticket personally, but as an FYI once #43055 is merged we are going to start leveraging webpack where we can in #43731

Whether the switch to webpack and refreshing the patch for this ticket helps or hinders ticket progress I'm unsure but hopefully helps and hopefully can support what you've noted above @WraithKenny in switching from CommonJS environment to a environment Browser for CodeMirror

[WraithKenny]

@netweb Yes, I guess my point is that using Webpack for packaging CodeMirror should be avoided as it makes it so it can't be extended at all in the browser.

Also, while creating a gulp build that concatenates the files for the browser environment, I noticed 2 addons are bundled that definitely should be avoided:

1. runmode/runmode-standalone.js
2. tern/worker.js

tern/worker.js appears to be a demo script that adds globals for a 3rd party (same author as CodeMirror) library integration for example purposes.

runmode-standalone.js shouldn't be bundled, as it's a standalone syntax highlighter (as opposed to editor). It's useful as a very light-weight highlighter that reuses CodeMirror's mode files, but it should be separated, not bundled (will cause errors).

[WraithKenny]

Just as a real world example, I have created this build script ​https://github.com/unFocus/Scripts-n-Styles/blob/c3bd165117f1a60d60823c1c315667bc876898ef/gulpfile.babel.js#L126-L196

in order to enable this line: ​https://github.com/unFocus/Scripts-n-Styles/blob/c3bd165117f1a60d60823c1c315667bc876898ef/dist/js/global-page.babel.js#L66

(of course I'm also including all of the modes individually in a folder in my plugin to make them available, and registering that folder via ​https://github.com/unFocus/Scripts-n-Styles/blob/c3bd165117f1a60d60823c1c315667bc876898ef/dist/js/global-page.babel.js#L8)

[WraithKenny]

An option to use Webpack, but not break extensibility of CodeMirror's browser mode: ​https://www.npmjs.com/package/webpack-merge-and-include-globally

[adamsilverstein]

punting to future release since we don't have a ready patch.

[desrosj]

This came up again with #48456. While this is not a blocker to update CodeMirror, it's preferable to change over to an automated process to make this easier going forward.

With that in mind, 41870.13.diff​ is an attempt to refresh against today's trunk, but is missing the changes to the Webpack files (I'm not that familiar with Webpack's innerworkings).

Looking at the changes, I also wonder if adding some of these packages is necessary since there have been a lot of changes to the build tooling in 2 years.

Some other notes:

• The required PR mentioned above in 41870#comment:21 has been merged and released in HTMLhint.
• The current version of CodeMirror is 5.49.2.
• Do we want to use more strict semver versions, or will relying on the package-lock.json file be enough? There is no consistency with this in the package.json file.

[adamsilverstein]

Thanks for the patch @desrosj! I'm going to review this for 5.4.