Replace browserify/gulp with Webpack #5918

sminnee · 2016-08-28T04:37:27Z

Details in the commits, feedback welcome!

Pull requests:
silverstripe/silverstripe-cms#1589
silverstripe/silverstripe-asset-admin#249
silverstripe/silverstripe-testsession#41
silverstripe/silverstripe-reports#48
silverstripe/silverstripe-simple#42

TODO

Notes

npm run sanity has been removed, because the manual copying of libraries from src to dist is, by and large, not needed. TinyMCE is still included from framework/thirdparty

stevie-mayhew · 2016-08-28T21:13:13Z

One think I notice is that it appears there isn't an explicit set to NODE_ENV=production for production builds. Will have a browse through the rest of it today.

chillu · 2016-08-29T11:37:02Z

Hey Sam, this looks awesome!

Could we either move this to the open-sausages repo, or give me temporary commit rights on your fork? I could also send pull requests to your fork, but that seems a bit overkill :)

Converted asset-admin, JS seems to work, figured it's the best way to learn. I can't get the CSS to compile. I've had to expose the new FormBuilderModal via framework. Also had to npm-shrinkwrap in framework in order to get npm install behaving. Check new commits on the open-sausages framework fork.

https://github.com/open-sausages/silverstripe-framework/tree/webpack
https://github.com/open-sausages/silverstripe-cms/tree/webpack
https://github.com/open-sausages/silverstripe-asset-admin/tree/webpack

I got a bit confused by the expose calls hidden away in bundles/lib.js, particularly since webpack.config.js has the equivalent export statements (in order to share modules within multiple bundles produced by framework). Should we move this to webpack.config.js completely? See https://github.com/webpack/expose-loader

module: {
  loaders: [
    { test: require.resolve("react"), loader: "expose?React" }
  ]
}

We have to fix the 4.0 changelog referencing build tooling as well as docs references to browserify:
https://docs.silverstripe.org/en/4.0/contributing/build_tooling/
https://docs.silverstripe.org/en/4.0/developer_guides/customising_the_admin_interface/how_tos/extend_cms_interface/

chillu · 2016-08-30T12:01:21Z

@sminnee Done some more work on this (in the open-sausages branches). Kept your commits intact to make review easier, we'll need to squash a bit once you're happy with my modifications.

I now know why you didn't use loaders for the expose config - you need scripts to be loaded in an order dependant way - all good.

Added some TODOs to the PR description.

I've done a bit of testing, looks good in IE10 and Chrome (main interactions, no missing files, no console errors). A full Behat run should uncover things pretty well

chillu · 2016-08-30T22:13:13Z

Talked to Sam and decided to push my commits to sminnee instead of the open-sausages repo. Removed the cms and framework repos, kept the asset-admin one intact. Current work:

https://github.com/sminnee/silverstripe-framework/tree/webpack
https://github.com/sminnee/silverstripe-cms/tree/webpack
https://github.com/open-sausages/silverstripe-asset-admin/tree/webpack

flamerohr · 2016-08-30T22:13:30Z

client/src/legacy/UploadField_downloadtemplate.js

@@ -1,4 +1,4 @@
-window.tmpl.cache['ss-uploadfield-downloadtemplate'] = tmpl(
+tmpl.cache['ss-uploadfield-downloadtemplate'] = tmpl(


Could add window.tmpl like in UploadField_uploadtemplate.js for consistency, not a breaking change

sminnee · 2016-08-31T23:01:29Z

@chillu I think that the partial revert of the dist rebuild is a bit excessive. Having done a search through the code for <img> tags, I see the following:

framework/admin:

framework/admin/client/dist/images/spinner.gif (from CMSLoadingScreen.ss)

framework:

BBCodeParser references images but this code is broken, so I think it's safe to ignore.

cms:

cms/client/dist/images/dialogs/alert.gif (from Install_successfullyinstalled.ss)

installer, asset-admin, siteconfig, reports: none

chillu · 2016-09-01T11:41:23Z

@sminnee I've pushed an update (and squashed). It's less about <img> references, most of these images are referenced in SCSS files through url(). I've gone through each image in client/dist/images and admin/client/dist/images and checked for their use, and trimmed down accordingly. Everything that's left in there is actually in use (at least until Paul gets around doing more vectorised implementations). I've removed images from dist/ which were inlined into the SCSS via data-urls.

Added a few TODOs at the top. What's your take on copying and sanity checking (diffing) third party code? Ideally we can get proper ES6 exports for those, not sure if anything has changed since we last looked at those particular libs.

@unclecheese We've talked about updating build tooling docs for SS4 on Twitter the other day. I've had to rewrite some of this in this PR as part of the Webpack switch, and restructured it a bit while I was going as well. Wanna have a read? Does the webpack.config.js setup generally make sense to you?

See silverstripe/silverstripe-framework#5918

chillu · 2016-09-01T22:43:32Z

Talked a bit more with Sam offline:

Remove duplication in thirdparty, reference node_modules if possible
Keep TinyMCE bundled through tinymce_gzip.php for now, the way plugins are configured doesn't lend itself to bundling with webpack (you have to prevent the plugin JS from being loaded to remove behaviour, there's no configuration to disable an already loaded plugin)
Note why we removed spriting (new stuff should be webfonts)
Document how we use webfonts (SaaS, contact us if you want to add any)

We've been able to remove nearly a THOUSAND FRICKEN NPM PACKAGES by switching to Webpack (and simplifying the build, e.g. no more spriting and coffeescript processing) - a full third less.

Before

npm ls | wc -l
3189
du -hc node_modules
763M    total

After

npm ls | wc -l
2301
du -hc node_modules
687M    total

chillu · 2016-09-06T10:23:10Z

Further to the point of loading TinyMCE via the PHP GZIP script: An example of procedural plugin registration, and the corresponding add() API.

If we keep a list of pre-bundled default plugins in our own bundle-lib.js, we could change HtmlEditorConfig to output a <script> tag which calls the corresponding remove() API in TinyMCE on plugins which have been removed through PHP or YAML config. New plugins added through HtmlEditorConfig would be loaded as separate JS files by default, and use the procedural add()

See silverstripe/silverstripe-framework#5918

chillu · 2016-09-09T01:01:10Z

@flamerohr added a react-based mini-pattern-lib to frameworktest via silverstripe/silverstripe-frameworktest#22. Which means we should migrate this to Webpack as well

See silverstripe/silverstripe-framework#5918

chillu · 2016-09-12T10:43:57Z

Facebook has released a create-react-app boilerplate which comes with a default webpack.config.js - worth a look:

https://github.com/facebookincubator/create-react-app/blob/master/config/webpack.config.dev.js
https://github.com/facebookincubator/create-react-app/blob/master/config/webpack.config.prod.js

tractorcow · 2016-09-13T02:14:15Z

Noted that the cms popup menu is aligned wrong. I've added a bullet to the todos.

tractorcow · 2016-09-13T02:37:00Z

I got this error running npm run build

ERROR in ./admin/client/src/bundles/lib.js
Module not found: Error: Cannot resolve module 'blueimp-file-upload/jquery.fileupload-ui.js' in /Users/dmooyman/Sites/ss40webpack/framework/admin/client/src/bundles
 @ ./admin/client/src/bundles/lib.js 58:0-54

tractorcow · 2016-09-13T03:39:35Z

For some weird reason, ontabsbeforeactivate in LeftAndMain.ActionTabSet.js isn't being triggered anymore.

tractorcow · 2016-09-13T04:53:44Z

It's caused by https://bugs.jqueryui.com/ticket/9316.

widgetEventPrefix is empty, so onbeforeactivate is triggered, but not ontabsbeforeactivate.

I think it's a bit beyond me to figure out how it's being broken... I might need to leave it to a frontender. =(

chillu · 2016-09-13T21:10:58Z

@sminnee I think we’re hitting this for Webpack’s TextExtract loader: webpack-contrib/extract-text-webpack-plugin#179 - basically it silently fails to merge multiple entry points into a single CSS file. It works with a [name] placeholder, but then you end up with bundle-legacy.css etc.

I'll comment out all the require(*.scss) statements for now, and move them all into bundle.scss as a separate entry point - then we can use [name] - it'll only create one CSS file because the other ones don't reference any SCSS

sminnee · 2016-09-13T21:23:20Z

Yeah good approach to get this over the line. We can refine later if we want.

tractorcow · 2016-09-14T00:05:38Z

I've had to back-port the above bugfix into our copy of jquery-ui.js, which is currently at 1.9. I attempted to upgrade to ui 1.10.4, but this would require us to re-do all our code which relied on .data() behaviour from 1.8, which is a breaking change.

tractorcow · 2016-09-14T05:27:56Z

I've fixed pretty much all that I can... the only remaining issue blocking tests passing is the framework/client/src/styles/legacy not being included in the dist css files.

See silverstripe/silverstripe-framework#5918

When adding the deps straight into the file (recommended), the onchange handler in file-upload isn't firing properly when a file is uploaded through the <input type=file> button - it falls back to default behaviour, which submits the containing form, failing because the upload is handled by a different URL.

chillu · 2016-09-15T11:09:34Z

OMG, I think this is actually ready to merge now! Behat and PHPUnit tests are passing locally for me. There's one test failure which is due to changed paths in the "simple" theme (silverstripe/silverstripe-simple#42, InvalidArgumentException: File framework/thirdparty/jquery/jquery.js does not exist).

sminnee · 2016-09-16T01:28:54Z

Forms/HTMLEditor/HTMLEditorField_Embed.php

@@ -106,7 +106,7 @@ public function getPreviewURL()
 		}

 		// Default media
-		return FRAMEWORK_DIR . '/images/default_media.png';
+		return FRAMEWORK_DIR . '/client/dist/images/src/default_media.png';


should we squash this back to "FIX Webpack handles images & fonts"?

It's a bit hard to tell which commit this belongs to, since that line has been changed in the PR afterwards - I'd say just leave it here, unless you can point out a commit

sminnee · 2016-09-16T01:34:33Z

OK I've reviewed Ingo's changes to this and I'm happy, assuming the tests pass.

sminnee · 2016-09-16T01:40:35Z

admin/client/src/legacy/ModelAdmin.js

@@ -3,6 +3,8 @@
 */


I can't see how this file is actually getting included, and my suspicion is that the code is so edge-case that tests don't file if we remove it entire.

Can we see some evidence that the .cms-content-tools #Form_SearchForm entwine rule has been bundled?

If not, do we need to include these files in the bundle-legacy entrypoint?

Good point, I've added it to legacy.js now and verified that the "expand csv stuff" feature works in ModelAdmin (so the JS kicks in)

There's not a lot of benefit in packaging these separately in terms of initial CMS load size, so let's simplify the setup. They'll eventually become lazy loaded chunks in a React-based setup

Used in iframe, so can't be rolled into other code. Moved in admin/ since that's easier for now with Webpack entry points, and we'll move all JS/CSS into admin/ anyway soon.

We've removed the ability to directly reference JS and CSS files for form fields and other SilverStripe features in favour of a common bundle built by Webpack. The logical next step is to make the framework module free of frontend dependencies, which should simplify its operation, and avoid another time intensive "npm install" on a module.

The JavaScript i18n functionality in SilverStripe is used in the CMS as well as form field implementations. Form fields used to include their own JavaScript for usage outside of CMS. This now requires custom build tooling in a project. Hence there's no need for an i18n shim (i18nx.js), since the CMS always uses i18n support.

Storing in fonts/, not font/fonts

Multiple entry points can't result in a single bundle.css with a fixed filename, see webpack-contrib/extract-text-webpack-plugin#179 Until that's resolved, it's easier to keep the 'css' task separate in Webpack, and have a single entry point for all CSS (bundle.scss). Also partially reverting "Moved frontend assets into admin/ "module"", which moved too many files: debug.css and install.css need to remain as framework (not admin) deps. Split out into a separate `framework-css` Webpack task in preparation for splitting off the module.

We don't have an easy way to copy over those files automatically into the thirdparty/ dir, where they are accessible on the webroot (not the case in node_modules). To avoid confusion, we don't duplicate into node_modules for now. See #5918 (comment)

They were used for TreeDropdownField JavaScript tests but were never run on CI, hence have been a bit of a wasted effort. We're using a different JS unit testing solution now (geared towards React), and running Jasmine in parallel on Travis for a few test cases doesn't warrant the setup effort involved. The TreeDropdownField component will eventually move to a React solution which can be developed in a test-driven fashion.

The 'admin' module will be split off from 'framework', where 'framework' only provides (mostly) frontend-agnostic PHP classes. For example, HTMLEditorField.php has a TinyMCEConfig.php driver, but doesn't come with its own JS includes.

When rewriting the i18n.js file from ES5 to ES6, the detectLocale() call in the constructor was missed - meaning the lang files were loaded by the browser, but never actually used.

We're no longer copying JS sources around, hence there's no need for this task.

sminnee · 2016-09-16T01:46:36Z

OK all tests except the CMS ones pass. @tractorcow do you want to merge this?

tractorcow · 2016-09-16T02:19:49Z

I've merged cms + testsession, restarting framework build.

sminnee · 2016-09-16T02:31:08Z

@tractorcow we're green. :-) I'm merging

See silverstripe/silverstripe-framework#5918

sminnee mentioned this pull request Aug 28, 2016

Replace browserify/gulp with Webpack silverstripe/silverstripe-cms#1589

Merged

flamerohr reviewed Aug 30, 2016
View reviewed changes

chillu added a commit to open-sausages/silverstripe-asset-admin that referenced this pull request Sep 1, 2016

Switch build system to webpack

93c972c

See silverstripe/silverstripe-framework#5918

chillu added a commit to open-sausages/silverstripe-asset-admin that referenced this pull request Sep 7, 2016

Switch build system to webpack

9ead1b0

See silverstripe/silverstripe-framework#5918

chillu mentioned this pull request Sep 9, 2016

Added TestReactFormBuilder admin section silverstripe/silverstripe-frameworktest#22

Merged

chillu added a commit to open-sausages/silverstripe-asset-admin that referenced this pull request Sep 10, 2016

Switch build system to webpack

811b783

See silverstripe/silverstripe-framework#5918

marcello-silverstripe added this to the CMS 4.0.0-alpha2 milestone Sep 12, 2016

chillu mentioned this pull request Sep 12, 2016

Tabs and CompositeField added to React formfields #5964

Merged

tractorcow mentioned this pull request Sep 13, 2016

Replace browserify/gulp with Webpack silverstripe/silverstripe-asset-admin#249

Merged

chillu pushed a commit to sminnee/silverstripe-cms that referenced this pull request Sep 14, 2016

FIX Switch gulp JavaScript generation to Webpack

beef8fa

See silverstripe/silverstripe-framework#5918

chillu added a commit to open-sausages/silverstripe-asset-admin that referenced this pull request Sep 14, 2016

Switch build system to webpack

b572fd4

See silverstripe/silverstripe-framework#5918

chillu added a commit to open-sausages/silverstripe-testsession that referenced this pull request Sep 14, 2016

Changed debug.css paths

a54ddb5

See silverstripe/silverstripe-framework#5918

chillu mentioned this pull request Sep 15, 2016

NEW: Run JS/CSS linting in Travis. #6016

Merged

1 task

sminnee closed this Sep 16, 2016

sminnee reopened this Sep 16, 2016

sminnee commented Sep 16, 2016

View reviewed changes

chillu and others added 14 commits September 16, 2016 13:44

Removed ModelAdmin/SecurityAdmin Webpack entry points

25f4741

There's not a lot of benefit in packaging these separately in terms of initial CMS load size, so let's simplify the setup. They'll eventually become lazy loaded chunks in a React-based setup

Entry point for UploadField_select.js

6be25f3

Used in iframe, so can't be rolled into other code. Moved in admin/ since that's easier for now with Webpack entry points, and we'll move all JS/CSS into admin/ anyway soon.

Move incorrect lang file location

e5df794

FIX Manually fix issue in jquery-ui 1.9

abaebbe

Removed unused dist files

8e5dd58

Storing in fonts/, not font/fonts

Removed TinyMCE NPM dep

686c167

We don't have an easy way to copy over those files automatically into the thirdparty/ dir, where they are accessible on the webroot (not the case in node_modules). To avoid confusion, we don't duplicate into node_modules for now. See #5918 (comment)

FIx i18n JS regression about locale selection

30174db

When rewriting the i18n.js file from ES5 to ES6, the detectLocale() call in the constructor was missed - meaning the lang files were loaded by the browser, but never actually used.

Removed unused "npm run sanity"

556667c

We're no longer copying JS sources around, hence there's no need for this task.

Updated dist files

3d7c60f

chillu approved these changes Sep 16, 2016

View reviewed changes

sminnee merged commit a1c9de3 into silverstripe:master Sep 16, 2016

sminnee pushed a commit to silverstripe/silverstripe-asset-admin that referenced this pull request Sep 16, 2016

Switch build system to webpack (#249)

0fd0d76

See silverstripe/silverstripe-framework#5918

tractorcow deleted the webpack branch September 16, 2016 02:33

dhensby mentioned this pull request Sep 19, 2016

[SS 4] BUG thirdparty/jquery throws invalid argument exception silverstripe/silverstripe-simple#43

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace browserify/gulp with Webpack #5918

Replace browserify/gulp with Webpack #5918

sminnee commented Aug 28, 2016 •

edited by dhensby

Loading

stevie-mayhew commented Aug 28, 2016

chillu commented Aug 29, 2016

chillu commented Aug 30, 2016 •

edited

Loading

chillu commented Aug 30, 2016

flamerohr Aug 30, 2016

sminnee commented Aug 31, 2016

chillu commented Sep 1, 2016

chillu commented Sep 1, 2016 •

edited

Loading

chillu commented Sep 6, 2016

chillu commented Sep 9, 2016

chillu commented Sep 12, 2016

tractorcow commented Sep 13, 2016

tractorcow commented Sep 13, 2016

tractorcow commented Sep 13, 2016

tractorcow commented Sep 13, 2016 •

edited

Loading

chillu commented Sep 13, 2016

sminnee commented Sep 13, 2016

tractorcow commented Sep 14, 2016

tractorcow commented Sep 14, 2016

chillu commented Sep 15, 2016

sminnee Sep 16, 2016

chillu Sep 16, 2016

sminnee commented Sep 16, 2016

sminnee Sep 16, 2016

chillu Sep 16, 2016

sminnee commented Sep 16, 2016

tractorcow commented Sep 16, 2016

sminnee commented Sep 16, 2016

		@@ -1,4 +1,4 @@
		window.tmpl.cache['ss-uploadfield-downloadtemplate'] = tmpl(
		tmpl.cache['ss-uploadfield-downloadtemplate'] = tmpl(

Replace browserify/gulp with Webpack #5918

Replace browserify/gulp with Webpack #5918

Conversation

sminnee commented Aug 28, 2016 • edited by dhensby Loading

TODO

Notes

stevie-mayhew commented Aug 28, 2016

chillu commented Aug 29, 2016

chillu commented Aug 30, 2016 • edited Loading

chillu commented Aug 30, 2016

flamerohr Aug 30, 2016

Choose a reason for hiding this comment

sminnee commented Aug 31, 2016

chillu commented Sep 1, 2016

chillu commented Sep 1, 2016 • edited Loading

chillu commented Sep 6, 2016

chillu commented Sep 9, 2016

chillu commented Sep 12, 2016

tractorcow commented Sep 13, 2016

tractorcow commented Sep 13, 2016

tractorcow commented Sep 13, 2016

tractorcow commented Sep 13, 2016 • edited Loading

chillu commented Sep 13, 2016

sminnee commented Sep 13, 2016

tractorcow commented Sep 14, 2016

tractorcow commented Sep 14, 2016

chillu commented Sep 15, 2016

sminnee Sep 16, 2016

Choose a reason for hiding this comment

chillu Sep 16, 2016

Choose a reason for hiding this comment

sminnee commented Sep 16, 2016

sminnee Sep 16, 2016

Choose a reason for hiding this comment

chillu Sep 16, 2016

Choose a reason for hiding this comment

sminnee commented Sep 16, 2016

tractorcow commented Sep 16, 2016

sminnee commented Sep 16, 2016

sminnee commented Aug 28, 2016 •

edited by dhensby

Loading

chillu commented Aug 30, 2016 •

edited

Loading

chillu commented Sep 1, 2016 •

edited

Loading

tractorcow commented Sep 13, 2016 •

edited

Loading