Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: Prerender using webpack #71

Merged
merged 53 commits into from
Jun 28, 2017

Conversation

rkostrzewski
Copy link
Collaborator

Prerender fails when webpack specific stuff is in js code (described here #43, here #39, here #62 and possibly other places).

This fixes that by creating separate webpack bundle just for prerender with commonjs2 target.

This adds some overhead when building, but I couldn't use webpack's multiple compiler with two configs because some plugins failed. 😢

WIP because I must fully test this & wanted to hear early feedback.

@developit
Copy link
Member

developit commented May 29, 2017

A thought I had: could we produce a UMD bundle that would be usable in both the client and the server? Definitely interested in any way to avoid the overhead of 2 concurrent builds. I think the closest prior art I know of here would be static-site-generator-webpack-plugin - maybe their technique would work. Or maybe a webpack bundle that can detect if its running under node/cjs and do a module.exports, otherwise performs a client render.

newConfig.output.libraryTarget = 'commonjs2';

let asyncLoaderIndex = newConfig.module.loaders
.findIndex(l => l.loader === path.resolve(__dirname, 'async-component-loader'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be able to just alias this to a dummy / passthrough loader

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it would be better + whenever someone uses import Component from 'async!./components/...' currently it won't work

@developit
Copy link
Member

developit commented May 29, 2017

If you don't mind I might also take a stab at this with the UMD approach. I'm curious, but it might not work at all so I don't want to waste your time 😇

@developit developit mentioned this pull request May 29, 2017
@rkostrzewski
Copy link
Collaborator Author

@developit go ahead! 😄

Also, I'm thinking about using some in memory file system (memfs for example) for webpack builds so that there won't be some magic folder called in npm module (+using normal fs breaks concurrency) 😄

@rkostrzewski
Copy link
Collaborator Author

@developit I didn't see your first comment.

A thought I had: could we produce a UMD bundle that would be usable in both the client and the server? Definitely interested in any way to avoid the overhead of 2 concurrent builds. I think the closest prior art I know of here would be static-site-generator-webpack-plugin - maybe their technique would work. Or maybe a webpack bundle that can detect if its running under node/cjs and do a module.exports, otherwise performs a client render.

Technically possible. Passing two configs to webpack with two entries (sth like webpack([{ entry: 'index.js', rules: ... }, { entry: 'App.js', rules: ... }]) doesn't add overhead and webpack doesn't seem to process same file twice (at least based on time it takes to compile). However, there is a problem with some plugins (just a guess) that are applied twice and throw errors. I want to investigate that and possibly exclude those plugins in SSR build.

@rkostrzewski
Copy link
Collaborator Author

I've been able to pinpoint location of the erroneous plugin 😄, this: goldhand/sw-precache-webpack-plugin#78 will enable us to use single webpack compiler pass & reduce overhead. 🎉

@rkostrzewski rkostrzewski changed the title [WIP] feature: Prerender using webpack feature: Prerender using webpack May 31, 2017
@rkostrzewski
Copy link
Collaborator Author

All wrapped up & tested.
Chosen not to use any memory filesystem (require doesn't work) and webpack creates ssr-build folder with bundle for SSR - this will be convenient when we will be working for server-side integration of rendering routes.

@developit if you haven't started yet & have some ideas for further work on this, feel free to work on this branch. 😄

I'm thinking about writing some tests that would be able to verify the CLI works correctly (not only running builds, but running built bundles & watch mode).

@lukeed
Copy link
Member

lukeed commented May 31, 2017

@rkostrzewski I think we should bump the sw-precache dependency now that your PR there has been merged.

@kurtextrem
Copy link

I haven't tried the code yet, but is polyfilling history and location enough? Where do the other things Preact uses (such as document.createElement) come from?

@developit
Copy link
Member

developit commented Jun 1, 2017

@kurtextrem During SSR, only h() is actually invoked from Preact. It knows to avoid any DOM mutation :)

@rkostrzewski I didn't get a chance to test out yet, so I'll just check this branch out and try some things.


customConfig({
resolve: {
modules: [
'node_modules',
resolve(__dirname, '../../node_modules')
resolve(__dirname, '../../../node_modules')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should just be using NODE_PATH for this wherever possible. Actually, anywhere we have node_modules references for resolving, we should add NODE_PATH. That'll make us future proof.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a variable inside the file const NODE_PATH = resolve(__dirname, '../../node_modules') or via CLI?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought NODE_PATH pointed to the global node_modules directory?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@developit Correct, which is what you want. It's basically used when people decide not to put node_modules inside their root dir but rather somewhere else.

Copy link
Member

@developit developit Jun 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right but that assumes global - preact-cli installs itself as a local dep and this is looking there. I had wondered if there was a way to get to that path... maybe something like:

path.join(require.resolve('preact-cli'), 'node_modules')

@prateekbh
Copy link
Member

just an fyi, i just debugged
#42 is also being caused due to prerender, i am trying to dig deeper, will post if any finding comes up

@hassanbazzi
Copy link
Member

I just tried this branch on my site and I got the following error:

(undefined) Template execution failed: Error: Cannot find module '/Users/mypath/build/ssr-build/ssr-bundle.js'
(undefined)   Error: Cannot find module '/Users/mypath/build/ssr-build/ssr-bundle.js'
  
  - module.js:20 require
    internal/module.js:20:19
  
  - prerender.js:17 prerender
    [website]/[preact-cli]/lib/lib/webpack/prerender.js:17:10
  
  - webpack-client-config.js:156 Object.ssr
    [website]/[preact-cli]/lib/lib/webpack/webpack-client-config.js:156:54
  
  - template.html:109 EM6I.module.exports
    [website]/[preact-cli]/lib/resources/template.html:109:37
  
  - index.js:265 
    [website]/[html-webpack-plugin]/index.js:265:16
  
  - util.js:16 tryCatcher
    [website]/[html-webpack-plugin]/[bluebird]/js/release/util.js:16:23
  
  - promise.js:512 Promise._settlePromiseFromHandler
    [website]/[html-webpack-plugin]/[bluebird]/js/release/promise.js:512:31
  
  - promise.js:569 Promise._settlePromise
    [website]/[html-webpack-plugin]/[bluebird]/js/release/promise.js:569:18
  
  - promise.js:606 Promise._settlePromiseCtx
    [website]/[html-webpack-plugin]/[bluebird]/js/release/promise.js:606:10
  
  - async.js:138 Async._drainQueue
    [website]/[html-webpack-plugin]/[bluebird]/js/release/async.js:138:12
  
  - async.js:143 Async._drainQueues
    [website]/[html-webpack-plugin]/[bluebird]/js/release/async.js:143:10
  
  - async.js:17 Immediate.Async.drainQueues
    [website]/[html-webpack-plugin]/[bluebird]/js/release/async.js:17:14

@hassanbazzi
Copy link
Member

LGTM! I just went through and tested this super thoroughly. Also tried using the few config options differently for templates and such. :)

# Conflicts:
#	package.json
#	tests/build.snapshot.js
#	tests/build.test.js
#	tests/lib/chrome.js
#	tests/lib/cli.js
#	tests/lib/output.js
#	tests/serve.test.js
# Conflicts:
#	src/lib/webpack/webpack-base-config.js
# Conflicts:
#	src/lib/webpack/webpack-base-config.js
# Conflicts:
#	src/lib/webpack/webpack-base-config.js
@hassanbazzi
Copy link
Member

teaaaam let's put this in master yo

@ethanroday
Copy link
Contributor

@hassanbazzi +1 but looks like tests are failing?

@rkostrzewski
Copy link
Collaborator Author

Seems like snapshots have different sizes on CI.

# Conflicts:
#	package.json
#	tests/build.snapshot.js
#	tests/build.test.js
# Conflicts:
#	package.json
#	src/commands/build.js
#	src/commands/watch.js
#	src/lib/webpack/webpack-base-config.js
#	tests/build.snapshot.js
#	tests/build.test.js
test(`preact build - should use custom .babelrc.`, options, async t => {
// app with custom .babelrc enabling async functions
let app = await fromSubject('custom-babelrc');

// UglifyJS throws error when generator is encountered
// TODO: Remove '--no-prederender' once #71 is merged - babel-register problem
await build(app, ['--no-prerender']);
await build(app);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Got it to work with uglify?

@hassanbazzi
Copy link
Member

LGTM :)

@hassanbazzi hassanbazzi merged commit aaebfaf into preactjs:master Jun 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants