Make WordPress Core

Opened 4 years ago

Last modified 22 months 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: Future Release Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch early needs-testing needs-refresh
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 (13)

41870.diff (2.5 MB) - added by adamsilverstein 4 years ago.
41870.2.diff (3.1 MB) - added by adamsilverstein 4 years ago.
41870.3.diff (3.1 MB) - added by adamsilverstein 4 years ago.
41870.4.diff (3.1 MB) - added by adamsilverstein 4 years ago.
41870.5.diff (3.5 MB) - added by adamsilverstein 4 years ago.
41870.6.diff (3.5 MB) - added by adamsilverstein 4 years ago.
41870.7.diff (4.2 MB) - added by adamsilverstein 4 years ago.
41870.8.diff (5.2 MB) - added by adamsilverstein 4 years ago.
41870.9.diff (4.6 MB) - added by adamsilverstein 4 years ago.
41870.10.diff (3.1 MB) - added by adamsilverstein 4 years ago.
41870.11.diff (7.2 MB) - added by adamsilverstein 4 years ago.
41870.12.diff (7.3 MB) - added by adamsilverstein 4 years ago.
41870.13.diff (1.3 MB) - added by desrosj 2 years ago.

Change History (67)

#1 @westonruter
4 years ago

  • Description modified (diff)

#2 @westonruter
4 years ago

  • Description modified (diff)

#3 @westonruter
4 years 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
4 years ago

  • Type changed from enhancement to task (blessed)

#5 @westonruter
4 years 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
4 years ago

  • Focuses javascript added

#7 @westonruter
4 years ago

  • Component changed from External Libraries to Build/Test Tools

#8 @westonruter
4 years ago

This also needs to be done for MediaElement.js

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

4 years ago

#10 @westonruter
4 years ago

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

#11 @adamsilverstein
4 years 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
4 years ago

41870.3.diff add codemirror to uglify for build

#14 follow-up: @adamsilverstein
4 years 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
4 years 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
4 years 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
4 years ago

41870.4.diff remove the watch for dev build

#18 @westonruter
4 years ago

  • Keywords dev-feedback removed

As we discussed on Slack, the next steps are:

#19 @adamsilverstein
4 years 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
4 years ago

41870.5.diff :

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

#21 @westonruter
4 years 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 😁):


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
4 years 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
4 years 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.

4 years ago

#25 @adamsilverstein
4 years 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
4 years ago

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

#27 @westonruter
4 years 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
4 years 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.

4 years ago

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

4 years ago

#31 @adamsilverstein
4 years 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
4 years 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
4 years 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 years ago

#35 @westonruter
4 years 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 years 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 years ago

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

Yeah, probably a good call.

#38 @westonruter
4 years ago

  • Priority changed from high to normal

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

4 years ago

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

4 years ago

#41 @westonruter
4 years ago

  • Description modified (diff)

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

#42 @adamsilverstein
4 years ago

41870.12.diff refresh against trunk.

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

4 years ago

#44 @WraithKenny
4 years ago

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


(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.)

Last edited 4 years ago by WraithKenny (previous) (diff)

#45 @netweb
4 years ago

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

#46 @WraithKenny
4 years ago

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

#47 @WraithKenny
4 years ago

Last edited 4 years ago by WraithKenny (previous) (diff)

#48 @jorbin
3 years ago

  • Milestone changed from 5.0 to 5.1

#49 @WraithKenny
3 years ago

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

#50 @adamsilverstein
3 years ago

  • Milestone changed from 5.1 to Future Release

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

2 years ago

#51 @desrosj
2 years ago

  • Keywords needs-refresh added

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.

#52 @adamsilverstein
2 years ago

  • Milestone changed from Future Release to 5.4

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

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

23 months ago

#54 @adamsilverstein
22 months ago

  • Milestone changed from 5.4 to Future Release
Note: See TracTickets for help on using tickets.