Use webpack to build final product #117

dmsnell · 2015-09-09T09:50:27Z

This patch changes the way the wpcom.js library is generated and built
as a library by incorporating a webpack-based build system.

Previously, when wpcom.js was imported into another project, the code
that was hand-written was what made it into the dependent project.
Previous coding style corresponded somewhat with this paradigm: skip
whitespace to save bytes.

Now, webpack is used to bundle up all of the constituent code, run it
through a processor, and spit out the final and single file. This opens
up new possibilities for the project including but not limited to:

Significant size reduction due to the way webpack optimizes and
minifies the output code. For example, the previous standalone code
was produced with browserify and was still a single file, but
that file was around 100 KB. The new bundle produced by webpack
is only 17 KB.
Function stripping: As a demonstration, for the standalone product I
have imported a plugin into webpack that strips out calls to
debug, console.log, and console.warn. This can be helpful for
making a general-release build without the development hooks.
ES6 running natively in the library! The output bundle ends up
pulling in a core-js shim, but we can use any ES6 functionality that
might want, including Promises, spread/rest operators, template
strings, destructuring, fat-arrow functions, and more.

Major changes

Version bump from 4.7.0 to 4.7.1 to reflect the fact that the
development and build system are completely new.
Updated package.json file with all required development
dependencies.
Developers no longer mess with index.js in the root project
directory. Instead, they should work in wpcom.js. The reason for
this is that when wpcom.js gets included into another project, it's
the index.js file that loads. Therefore, I chose to load in the
processed and bundled version instead of the human-formatted one.

Special notes

The webpack.config.js and webpack.config.dist.js files are
related. The dist file simply extends the base configuration to
produce something like what browserify used to create. While the
base configuration is set to create a commonjs2 module, the dist
bundle will still remain in the UMD format. Console logging has
also been removed from the standalone version.

Ironically, these files won't work with ES6 syntax, so I have
included the babel transform into the dist config so I can use
Object.assign().
It's important that we explicitly tell webpack which imported
modules are external libraries otherwise it will try to merge them in
and then we lose proper naming conventions and things break.
Before committing I tried testing with both the standalone and base
bundles. You can try this by editing test/util.js and changing the
top import statement to specify the exact file you want.

Questions

Is there a good reason to remove the console warnings in either the
standalone or the base build? A reason not to? I didn't think that
developers would appreciate the messages in the console, but maybe
they would want the debug() statements...
Is there much of a purpose in even having a dist standalone build anymore
since the root index.js is now a processed bundle? Of course, we don't have
to replace index.js from what it was, but I think this makes integration into
other projects better because they won't get the code bloat and they won't run
into issues with ES6 syntax.
We can build source maps for these files but I didn't in this commit. Do we want
them? I wouldn't see why we wouldn't, but since they didn't already exists I didn't
change it.

cc: @retrofox @TooTallNate

dmsnell · 2015-09-09T09:51:43Z

package.json

@@ -19,12 +20,16 @@
  "license": "MIT",
  "dependencies": {
    "debug": "^2.1.3",
+    "fs": "0.0.2",


As a general note - this line here was missing on the earlier releases and caused a problem when I tried to use the latest 4.7.0 release in another project. It needs to be merged into master regardless of this PR.

dmsnell · 2015-09-09T11:16:58Z

I tested within another project and called out for this branch specifically in the package.json and things worked really well.

aduth · 2015-09-23T15:20:55Z

webpack.config.dist.js

@@ -0,0 +1,23 @@
+require("babel/register");


Here and in a couple other places in the same file, there's some inconsistency on quotes and spacing around parentheses. The pattern I'm seeing elsewhere in the code base is single quotes, no spaces between parentheses.

Some of this is lazy because I have another PR sitting out there to reformat everything using esformatter. By the time I was writing this PR I assumed the formatting one would be merged already or that they would come at the same time.

I fixed this case and will try and fix other places as well, thanks @aduth for your input!

dmsnell · 2015-10-02T23:41:54Z

@aduth + @retrofox

After lots of stewing and testing, I have decided to release this in the
following manner:

- All production files are built into the `dist/` directory
- The node file is `index.js`
- The browser file is `wpcom.js`
- Both files are minified and have source-maps included.
- The node version depends on the `npm` dependencies
- The browser version only depends on the ES6 shims, such as Babel's
  polyfill or `core.js`.

To test the browser version I created an HTML file just like in
Readme.md and included the minified wpcom.js. First, however, I
introduced a fatal error in the library and rebuilt the release files.
When the Exception appeared in my browser, I was able to click on it in
Chrome's DevTools and see the original source code via the source maps.

I believe that this should be sufficient for our purposes here in that
others are still welcome to use this without node and with the source
map and without requiring a huge file.

- Minified node build (no libraries included) : 16 KB
- Minified browser build (with libraries) : 38 KB

aduth · 2015-10-05T13:01:26Z

What benefit is there to minifying the server-side variant?

When you say the browser version depends on shims, are those included in the compiled version? It's important that a user be able to easily drop in the script without much effort.

dmsnell · 2015-10-05T15:35:20Z

@aduth: There's not too much benefit to the node build to minimize, but we do need to run it through Babel if we don't want people having issues with the Promise code.

In my tests, I ran webpack to bundle things and pass them through Babel. The result of this was some ridiculous source code because of the way webpack transformed the variable names __webpack_var(15) stuff like that. The other option is to run Babel on everything individually and spit out a new distribution directory, but I thought that was somewhat silly to do to create all those duplicate files, so I stuck with webpack.

Concerning core.js I chose not to include it and instead specify that it's required in the Readme.md file. One reason is for the build size, but the other reason is that core.js, or whichever other shim someone uses, is something probably a bit more dynamic than wpcom.js, so I figured we wouldn't want to combine them. This still doesn't require much effort if you look at the Readme. It's akin to requiring that someone load jQuery.

Feel free to explore this issue yourself and commit here. I'm probably at the end of what I can productively offer concerning the npm package.

aduth · 2015-10-06T12:35:50Z

Took another pass over the changes. A few thoughts:

Don't we need to declare browser in the package.json for this file to be used in browser contexts?
Is Webpack being used to build the server-side build, and if so, should it be? I've noticed other projects using babel directly for the server-side build.
Can we make promises opt-in so as not to force the additional dependency? In other words, is there a way to detect whether the current environment supports promises, and prevent executing any related code if it's not supported?

dmsnell · 2015-10-06T16:52:08Z

@aduth:

I don't think we need browser in the package.json. It appears to be there for replacements that need to occur when building for a browser, but our code runs 100% in the browser.

Like I mentioned above I'm running webpack on both builds in order to bundle them. This produces a single output file instead of one for every source file. When I experimented with Babel anyway, it didn't change much (since we don't have much ES6 in our code) other than introduce the Babel shim dependency. The point of Babel is mainly to go from ES6 to ES5 syntax, whereas the point of webpack is to bundle and minify. Webpack just happens to make it easy to run the code through Babel in that process. Feel free to contribute a change though - like I said, I'm not an expert.

One thing though is that we really don't have a server-side build. All of the builds are intended to be executed in the browser. The difference is that one is built as a CommonJS module to be loaded in a node environment and the other creates a var in global scope and has all the dependencies included. That's the biggest difference: will someone else's build process bring in the dependencies or not?

As for the promises, I don't see any way of making them optional. They are core JS spec now and browser adoption should follow. The only way to remove this dependency is to include something like the core-js shim in the bundle, which would make it grow pretty big.

aduth · 2015-10-07T13:12:42Z

Like I mentioned above I'm running webpack on both builds in order to bundle them. ... One thing though is that we really don't have a server-side build.

I guess this is what I was confused about. It's obviously fine to use Webpack for the browser build, but I wasn't sure it would work (or be recommended) to use it for the server build. I've seen other projects using Webpack for the server build, but the Babel CLI for server builds (i.e. babel src --out-dir lib).

dmsnell · 2015-10-07T22:23:36Z

@retrofox: @aduth and I had a conversation in Slack earlier about how best to package the node-specific build of wpcom.js. We have essentially two options and we would appreciate some decision-making on your part 😄

Produce a single-file bundle via webpack

This is the way I originally wrote up the PR and it uses the same pipeline for the browser-builds and the node-builds. The output is a single minified file and the accompanying source map.

Run the individual library files through Babel

This is the way @aduth proposed and it copies all of the source files into a distribution directory, running each one through Babel. The output isn't minified and there are no source maps because the individual files differ from the original source only in that Babel has translated any ES6 features to ES5.

Point and Counterpoint

@aduth was concerned about the webpack build messing up the way the library works in node projects, since libraries like superagent behave differently in the different environments. I responded by mentioning that this doesn't impact wpcom.js because the webpack build considers all dependencies as externals and won't pack them. That means superagent is completely separate. I ran the full unit-test suite using the original source code, the minified browser build, and the minified node-build and all three passed 100%.

There remains then probably a philosophical distinction: for node environments where people will presumably have a build system, do we provide the single minified file or the directory of un-minified files (16 in total).

Neither one of us felt confident making a decision wherein we seem to prefer opposite methods. Do you have thoughts on this and could you guide the final PR organization? We are both fine going either direction.

dmsnell · 2015-10-22T23:30:05Z

@aduth I have followed your recommendations here as best as I understand them and I think they are probably the best way to move forward here. In any case, I think we need to move forward because we still have a broken release out there to the public.

As far as I can tell, running the code through Babel does not remove the need for the browser shim for promises. That will still be a requirement for the library, but I have that listed in the documentation in this PR.

@retrofox do we think we can get this vetted and merged soon?

P.S. The circleCI errors relate to the now-removed me/connections endpoint. I'll try and get that cleaned up pronto.

aduth · 2015-10-23T13:56:01Z

Do we need to add new make targets to .PHONY ?
I see the following in my browser console when loading the dist copy: I'm fs modules
dist/lib, dist/util, and dist/test should probably be added to .gitignore
dist/wpcom.js and dist/wpcom.js.map could optionally be added to a .npmignore
Why is util not included by default in dist/lib make target?
For whatever reason, the prepublish script was being run when I installed via npm install

All that said, I am liking the new structure of dist !

mattsherman · 2015-11-02T21:56:31Z

@dmsnell @retrofox @aduth Is there anything I can do to help assist moving this forward to completion? I'm waiting on this.

Let me know if there are things I can help out with so that I can move forward with the above. Thanks!

/cc @jonathansadowski @jblz

dmsnell · 2015-11-02T22:00:45Z

@mattsherman if you want to take a look at @aduth's comments and push up a commit that would definitely help. I apologize because although I saw his comments ten days ago, I forgot about them since then.

This is probably harder than it needs to be because we're doing two things: adding promises; and overhauling the build process into npm from Make

mattsherman · 2015-11-03T15:08:13Z

@dmsnell I'll look into things and push a commit that addresses the outstanding issues.

dmsnell · 2015-11-03T19:35:22Z

Thanks @mattsherman - let me know if you have any questions - I'd be happy to help out.

I think the highest priority right now would be to sit down and get the interactions between the npm scripts and the Makefile proper. Right now it's a strange mixture of the two methodologies; for example, I ran into trouble because the postinstall script which Babelified the library ended up calling npm install via the Makefile which then called postinstall creating a loop.

This patch changes the way the `wpcom.js` library is generated and built as a library by incorporating a **webpack**-based build system. Previously, when `wpcom.js` was imported into another project, the code that was hand-written was what made it into the dependent project. Previous coding style corresponded somewhat with this paradigm: skip whitespace to save bytes. Now, webpack is used to bundle up all of the constituent code, run it through a processor, and spit out the final and single file. This opens up new possibilities for the project including but not limited to: - Significant size reduction due to the way **webpack** optimizes and minifies the output code. For example, the previous standalone code was produced with **browserify** and was still a single file, but that file was around 100 KB. The new bundle produced by **webpack** is only 17 KB. - Function stripping: As a demonstration, for the standalone product I have imported a plugin into **webpack** that strips out calls to `debug`, `console.log`, and `console.warn`. This can be helpful for making a general-release build without the development hooks. - ES6 running natively in the library! The output bundle ends up pulling in a core-js shim, but we can use any ES6 functionality that might want, including Promises, spread/rest operators, template strings, destructuring, fat-arrow functions, and more. - Version bump from 4.7.0 to 4.7.1 to reflect the fact that the development and build system are completely new. - Updated `package.json` file with all required development dependencies. - Developers no longer mess with `index.js` in the root project directory. Instead, they should work in `wpcom.js`. The reason for this is that when `wpcom.js` gets included into another project, it's the `index.js` file that loads. Therefore, I chose to load in the processed and bundled version instead of the human-formatted one. - The `webpack.config.js` and `webpack.config.dist.js` files are related. The `dist` file simply extends the base configuration to produce something like what **browserify** used to create. While the base configuration is set to create a `commonjs2` module, the `dist` bundle will still remain in the `UMD` format. Console logging has also been removed from the standalone version. Ironically, these files won't work with ES6 syntax, so I have included the **babel** transform into the `dist` config so I can use `Object.assign()`. - It's important that we explicitly tell **webpack** which imported modules are external libraries otherwise it will try to merge them in and then we lose proper naming conventions and things break. - Before committing I tried testing with both the standalone and base bundles. You can try this by editing `test/util.js` and changing the top import statement to specify the exact file you want. - Is there a good reason to remove the console warnings in either the standalone or the base build? A reason not to? I didn't think that developers would appreciate the messages in the console, but maybe they would want the `debug()` statements... Include source-maps Set the module entry point in package.json Previously in order to accomodate the combiled webpack bundle I had to rename the existing `index.js` into `wpcom.js` so that the right file would import when loading this module. I have now discovered the "main" attribute for `package.json` and have used that here instead of renaming `index.js`. Now, the build stage produces `index.min.js` which is the compiled output that other scripts will load when they `require( 'wpcom' )` Catch up to changes in master

mattsherman · 2015-11-04T18:31:16Z

Rebased against master and added commits to address most of @aduth's comments... still need to test more and fix a failing test case (pretty sure this was failing before):

Do we need to add new make targets to .PHONY ?

Done.

I see the following in my browser console when loading the dist copy: I'm fs modules

Still need to look into and address this.

dist/lib, dist/util, and dist/test should probably be added to .gitignore

Done.

dist/wpcom.js and dist/wpcom.js.map could optionally be added to a .npmignore

Not addressed yet. If we do this, we'll have to maintain .gitignore and .npmignore -- do we want to do this?

Why is util not included by default in dist/lib make target?

Done.

For whatever reason, the prepublish script was being run when I installed via npm install

This is actually expected (okay, not really expected, but accepted) behavior... npm/npm#3059.

Removed prepublish script and added make publish target.

dmsnell · 2015-11-04T18:45:48Z

.gitignore

@@ -5,3 +5,6 @@ npm-debug.log
 /gh-tmp
 .DS_Store
 .idea
+/dist/lib
+/dist/util
+/dist/test


Can't we just add /dist/ here or are there files we want to keep?

If, for example we want to keep the standalone file...

/gh-tmp .DS_Store .idea /dist !/dist/wpcom.js

That ! should whitelist the file

Taken care of in 4a1cb85

dmsnell · 2015-11-04T18:50:58Z

@mattsherman I added a few comments here. Thanks for your work!

aduth · 2015-11-04T18:56:06Z

I see the following in my browser console when loading the dist copy: I'm fs modules

Still need to look into and address this.

Could it be the addition of the fs module to dependencies here? Do we need that? How about specifying fs: 'empty' in the Webpack config node option?

dmsnell · 2015-11-04T19:01:23Z

@aduth I added fs initially because wpcom.js broke without it. For some reason, it was actually a dependency but since it wasn't in the package.json file, it didn't get installed, thus the breakage.

Maybe I was too hasty and fixed the wrong problem, but we definitely need to make sure that we don't end up in the same situation.

retrofox · 2015-11-04T19:11:27Z

we need fs module for those methods that need upload a file, for instance media.addFiles, in server side. We could find a way to require fs only in server-side since it clearly isn't needed in client-side.

retrofox · 2015-11-04T19:17:29Z

How about specifying fs: 'empty' in the Webpack config node option?

it would be a half-court shot.

dmsnell · 2015-11-04T19:20:33Z

@aduth @mattsherman @retrofox maybe we set an environment variable in webpack. By doing this, we could ignore the fs code when building the standalone build.

Is that code distinct @retrofox? Webpack actually has a loader to strip out certain calls. If the fs code were all contained within a single function, then we could simply strip it out from the standalone build.

dist/index.js should be built when installed

…pcom.js into update/webpack-build-system

retrofox · 2015-11-05T13:14:59Z

Is that code distinct @retrofox?

What do you mean ? distinct server-side that client-side?
It's the same code, but there are some features that are handled in different way such as uploading a file. If wpcom.js is running in a browser (client-side) we have to use formData to upload files. If it's server-side we have to use fs module and its stream methods.

btw Cannot resolve module fs
pugjs/pug-loader#8 (comment)

retrofox · 2015-11-05T14:38:34Z

I've removed fs from package.json file and defined "empty" in webpack config file.
Tests server-side passed:

> make test wpcom.site.media

  wpcom.site
    wpcom.site.lists
      wpcom.site.mediaList
        ✓ should request media library list

  wpcom.site.media
    wpcom.site.media.get
      ✓ should get added media
    wpcom.site.media.update
      ✓ should edit the media title
    wpcom.site.media.addFiles
      ✓ should create a new media from a file (24029ms)
    wpcom.site.media.addUrls
      ✓ should create a new media from an object (2128ms)
    wpcom.site.media.addUrls

And also I've updated the ./examples/browser-proxy/upload.html example.

dmsnell · 2015-11-05T15:25:59Z

This must explain the problems I had getting it to run in node - the fact that it's only used in the browser.

@retrofox by distinct (which I realize was poorly worded) I meant was the code contained in its own function. If it were, it's pretty easy to replace that function at build-time. The alternative is that the platform-specific code is scattered among the universal code which would make it pretty ugly to pick apart.

Changed to use `wpcom-oauth-cors` to auth user. Need to put client id and site in `examples/browser-cors/upload-images.html` in order to work.

Use webpack to build final product

dmsnell added enhancement [Status] Needs Review labels Sep 9, 2015

dmsnell assigned retrofox Sep 9, 2015

dmsnell reviewed Sep 9, 2015
View reviewed changes

dmsnell force-pushed the update/webpack-build-system branch from dbc66fb to a042006 Compare September 9, 2015 10:19

dmsnell force-pushed the update/webpack-build-system branch from e8a2b04 to 91f1b0e Compare September 16, 2015 22:38

aduth reviewed Sep 23, 2015
View reviewed changes

dmsnell force-pushed the update/webpack-build-system branch from ecbb036 to ad8e82f Compare October 2, 2015 19:11

dmsnell force-pushed the update/webpack-build-system branch from 0b0783d to 3078938 Compare October 22, 2015 23:44

retrofox force-pushed the update/webpack-build-system branch 3 times, most recently from 6daec61 to ec2330e Compare November 3, 2015 23:40

dmsnell reviewed Nov 4, 2015
View reviewed changes

retrofox and others added 3 commits November 4, 2015 17:26

pkg: move babel into "dependencies" key

32a7a6a

Clean up .gitignore, remove dist/index.js

4a1cb85

dist/index.js should be built when installed

Merge branch 'update/webpack-build-system' of github.com:Automattic/w…

503466a

…pcom.js into update/webpack-build-system

dmsnell added this to the Build with Webpack milestone Nov 4, 2015

Added self to list of contributors

fe07656

retrofox added 3 commits November 5, 2015 11:07

remove fs from building process

793bcdd

example: update uploading testing file (proxy)

53498ba

dist:updated

8eba5e8

mattsherman and others added 3 commits November 5, 2015 16:57

Fixed examples/browser-cors/upload-images.html

7021e6f

Changed to use `wpcom-oauth-cors` to auth user. Need to put client id and site in `examples/browser-cors/upload-images.html` in order to work.

example: tweak upload images cors example

6cff4f1

example: rename uploading testing file

c77b18d

retrofox added a commit that referenced this pull request Nov 19, 2015

Merge pull request #117 from Automattic/update/webpack-build-system

aa6718a

Use webpack to build final product

retrofox merged commit aa6718a into master Nov 19, 2015

retrofox deleted the update/webpack-build-system branch November 19, 2015 13:15

retrofox added [Status] Ready to Merge and removed [Status] Needs Review labels Nov 19, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use webpack to build final product #117

Use webpack to build final product #117

dmsnell commented Sep 9, 2015

dmsnell Sep 9, 2015

dmsnell commented Sep 9, 2015

aduth Sep 23, 2015

dmsnell Sep 26, 2015

dmsnell Oct 1, 2015

dmsnell commented Oct 2, 2015

aduth commented Oct 5, 2015

dmsnell commented Oct 5, 2015

aduth commented Oct 6, 2015

dmsnell commented Oct 6, 2015

aduth commented Oct 7, 2015

dmsnell commented Oct 7, 2015

dmsnell commented Oct 22, 2015

aduth commented Oct 23, 2015

mattsherman commented Nov 2, 2015

dmsnell commented Nov 2, 2015

mattsherman commented Nov 3, 2015

dmsnell commented Nov 3, 2015

mattsherman commented Nov 4, 2015

dmsnell Nov 4, 2015

mattsherman Nov 4, 2015

dmsnell commented Nov 4, 2015

aduth commented Nov 4, 2015

dmsnell commented Nov 4, 2015

retrofox commented Nov 4, 2015

retrofox commented Nov 4, 2015

dmsnell commented Nov 4, 2015

retrofox commented Nov 5, 2015

retrofox commented Nov 5, 2015

dmsnell commented Nov 5, 2015

Use webpack to build final product #117

Use webpack to build final product #117

Conversation

dmsnell commented Sep 9, 2015

Major changes

Special notes

Questions

dmsnell Sep 9, 2015

Choose a reason for hiding this comment

dmsnell commented Sep 9, 2015

aduth Sep 23, 2015

Choose a reason for hiding this comment

dmsnell Sep 26, 2015

Choose a reason for hiding this comment

dmsnell Oct 1, 2015

Choose a reason for hiding this comment

dmsnell commented Oct 2, 2015

aduth commented Oct 5, 2015

dmsnell commented Oct 5, 2015

aduth commented Oct 6, 2015

dmsnell commented Oct 6, 2015

aduth commented Oct 7, 2015

dmsnell commented Oct 7, 2015

Produce a single-file bundle via webpack

Run the individual library files through Babel

Point and Counterpoint

dmsnell commented Oct 22, 2015

aduth commented Oct 23, 2015

mattsherman commented Nov 2, 2015

dmsnell commented Nov 2, 2015

mattsherman commented Nov 3, 2015

dmsnell commented Nov 3, 2015

mattsherman commented Nov 4, 2015

dmsnell Nov 4, 2015

Choose a reason for hiding this comment

mattsherman Nov 4, 2015

Choose a reason for hiding this comment

dmsnell commented Nov 4, 2015

aduth commented Nov 4, 2015

dmsnell commented Nov 4, 2015

retrofox commented Nov 4, 2015

retrofox commented Nov 4, 2015

dmsnell commented Nov 4, 2015

retrofox commented Nov 5, 2015

retrofox commented Nov 5, 2015

dmsnell commented Nov 5, 2015