Build: Add pragma check webpack plugin for server side rendering #1801

ehg · 2015-12-18T14:03:18Z

Closes #1594

Add a webpack plugin, for the client bundler, that:

Scans module sources for the ssr-ready pragma
Scans dependencies of these modules, and checks that they also have the ssr-ready pragma. Adds a webpack compilation error if not.

Did look like this, now looks like #1801 (comment)

TODO:

Better error messaging
Decide whether to bail on error
Some profiling on how this affects build performance (it subjectively feels it doesn't add much)
Remove test pragmas before merge

To test:

make run
Add /** @ssr-ready **/ to a file
Watch as the plugin reports unsatisfied constraints for that file's dependencies, and bails
Add the pragma to the listed dependencies, until make run doesn't error

mtias · 2015-12-18T16:52:42Z

Nice :)

ehg · 2016-01-07T16:19:45Z

Decided to bail the make run process if a pragma check error is found, this may be kind of annoying when encountered, but it's not going to happen too often at first. We can always investigate injecting something like alert( 'SSR errors' ) later, if necessary.

Currently looks like:

ehg · 2016-01-07T16:20:34Z

/cc @mcsf for a pre-'Needs Review' review :)

ehg · 2016-01-07T16:42:21Z

Some profiling on how this affects build performance (it subjectively feels it doesn't add much)

It doesn't seem to adversely affect build perf. I ran a few tests vs master, and the build times were pretty similar — this PR being faster for some tests, slower for others…

ockham · 2016-01-08T16:47:43Z

DevX concern -- if I screw up SSR and forget to restart Calypso, how will I know I messed up? Shouldn't there be some test that also fails? (In order to e.g. prevent merges via CircleCI.)

ehg · 2016-01-08T16:53:43Z

DevX concern -- if I screw up SSR and forget to restart Calypso, how will I know I messed up? Shouldn't there be some test that also fails? (In order to e.g. prevent merges via CircleCI.)

The make run process will fail, so hopefully you'd notice that. If you didn't, and there's a dependency error, it should be caught by make test's build rule dependency, so will error in CircleCI. If it's a more subtle mess up, #2240 will catch most renderToString errors, also in CircleCI.

ehg · 2016-01-08T17:00:51Z

The make run process will fail

Noting that we'll only fail it here if, say, a dependency not marked as ssr-ready is included in one that is. The purpose is to both document what modules we have deemed SSR-ready, but also to provide information and visibility when a 'risky' dependency is added.

It doesn't do anything clever, so won't catch any SSR-incompatible code errors. Maybe we could check for stuff like window being used and warn, but it's a bit of a rabbit hole.

ockham · 2016-01-08T17:05:46Z

server/pragma-checker/index.js

+
+	module.dependencies.forEach( function( dep ) {
+		// If the module is compiled through babel, we can be pretty sure it's our own module, not from npm.
+		if ( dep.module && /babel-loader/.test( dep.module.request ) &&


indexOf instead of the RegEx 'cause it's probably faster? </nitpick>

Sure :) lodash's includes should be fast enough: https://github.com/lodash/lodash/blob/3.10.1/lodash.src.js#L6707

ockham · 2016-01-08T17:29:53Z

Looks good -- nitpicks aside :-)

I don't really have a clue about webpack, but I was just wondering if we were duplicating module tree traversal (since I guess webpack needs to traverse the module tree for compilation anyway)? And if there was a way to just tack onto that on its way bottom-up, setting a per-module SSR flag, and check immediate child modules for the presence of that flag...

Anyway, better to have a check soon than to spend too much time over-engineering. Feel free to merge 👍

seear · 2016-01-11T10:51:20Z

server/pragma-checker/index.js

+		if ( dep.module && /babel-loader/.test( dep.module.request ) &&
+				dep.module._source &&
+				dep.module._source._value.indexOf( SSR_READY ) === -1 ) {
+			compilation.errors.push( PLUGIN_TITLE + ': ' + module.rawRequest + ', dependency ' + dep.module.rawRequest + ' is not ' + SSR_READY );


We might want a brief explanation of the SSR acronym in the output.

We might want a brief explanation of the SSR acronym in the output.

Ignore, I now see the next line of output 👍

seear · 2016-01-11T12:06:49Z

Am I right in thinking that the scanning algorithm means we can be looking for the pragma in the same module over and over again if it happens to be often used as a dependency often? If so we could cache results, but that is probably unnecessary complexity at the moment.

ehg · 2016-01-11T12:11:10Z

Am I right in thinking that the scanning algorithm means we can be looking for the pragma in the same module over and over again if it happens to be often used as a dependency often? If so we could cache results, but that is probably unnecessary complexity at the moment.

Yes, good point. I'll see what happens perf-wise when we have more pragmas.

seear · 2016-01-11T12:45:34Z

Looks good 👍

ehg · 2016-01-11T13:13:56Z

Removed test pragmas, and added a basic README.

Add a webpack plugin that: - Scans module sources for the ssr-ready pragma - Scans non-npm dependencies of these modules, and checks that they also have the ssr-ready pragma. Adds a webpack compilation error if not. - Bails the process if errors are found(!)

seear · 2016-01-11T14:11:36Z

Readme looks fine, good to merge 👍

Build: Add pragma check webpack plugin for server side rendering

To satisfy Automattic/wp-calypso#1801

ehg added [Feature Group] Appearance & Themes Features related to the appearance of sites. [Status] In Progress Build Server Side Rendering labels Dec 18, 2015

ehg self-assigned this Dec 18, 2015

ehg added this to the Themes: Showcase M3-LoggedOut milestone Dec 18, 2015

ehg mentioned this pull request Jan 6, 2016

Themes: Add server route for logged-out /design #2087

Merged

ehg force-pushed the add/webpack-ssr-checker branch from 9171379 to c03c9bf Compare January 7, 2016 16:16

ehg force-pushed the add/webpack-ssr-checker branch from c03c9bf to ae0f329 Compare January 7, 2016 16:23

ehg added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Jan 8, 2016

ehg force-pushed the add/webpack-ssr-checker branch from 2d643e8 to 7a80f51 Compare January 8, 2016 15:50

ockham reviewed Jan 8, 2016
View reviewed changes

seear reviewed Jan 11, 2016
View reviewed changes

ehg force-pushed the add/webpack-ssr-checker branch from 85e21a8 to 9a25324 Compare January 11, 2016 14:00

ehg added a commit that referenced this pull request Jan 11, 2016

Merge pull request #1801 from Automattic/add/webpack-ssr-checker

354cd5c

Build: Add pragma check webpack plugin for server side rendering

ehg merged commit 354cd5c into master Jan 11, 2016

ehg deleted the add/webpack-ssr-checker branch January 11, 2016 14:12

ehg removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 11, 2016

ehg added a commit to Automattic/gridicons that referenced this pull request Jan 11, 2016

Add SSR-ready pragma

3bf9834

To satisfy Automattic/wp-calypso#1801

ehg mentioned this pull request Jan 11, 2016

Add SSR-ready pragma Automattic/gridicons#136

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build: Add pragma check webpack plugin for server side rendering #1801

Build: Add pragma check webpack plugin for server side rendering #1801

ehg commented Dec 18, 2015

mtias commented Dec 18, 2015

ehg commented Jan 7, 2016

ehg commented Jan 7, 2016

ehg commented Jan 7, 2016

ockham commented Jan 8, 2016

ehg commented Jan 8, 2016

ehg commented Jan 8, 2016

ockham Jan 8, 2016

ehg Jan 8, 2016

ehg Jan 11, 2016

ockham commented Jan 8, 2016

seear Jan 11, 2016

seear Jan 11, 2016

seear commented Jan 11, 2016

ehg commented Jan 11, 2016

seear commented Jan 11, 2016

ehg commented Jan 11, 2016

seear commented Jan 11, 2016

Build: Add pragma check webpack plugin for server side rendering #1801

Build: Add pragma check webpack plugin for server side rendering #1801

Conversation

ehg commented Dec 18, 2015

mtias commented Dec 18, 2015

ehg commented Jan 7, 2016

ehg commented Jan 7, 2016

ehg commented Jan 7, 2016

ockham commented Jan 8, 2016

ehg commented Jan 8, 2016

ehg commented Jan 8, 2016

ockham Jan 8, 2016

Choose a reason for hiding this comment

ehg Jan 8, 2016

Choose a reason for hiding this comment

ehg Jan 11, 2016

Choose a reason for hiding this comment

ockham commented Jan 8, 2016

seear Jan 11, 2016

Choose a reason for hiding this comment

seear Jan 11, 2016

Choose a reason for hiding this comment

seear commented Jan 11, 2016

ehg commented Jan 11, 2016

seear commented Jan 11, 2016

ehg commented Jan 11, 2016

seear commented Jan 11, 2016