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

Fix chunks ordering in case of manifest + vendor files #115

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

izeau
Copy link

@izeau izeau commented Aug 28, 2017

Based on work done by @HorizonXP in #84 & @stephen-haddix in #64

  • Fixes ERROR in ReferenceError: webpackJsonp is not defined
  • Fixes ERROR in TypeError: Cannot read property 'call' of undefined

Coverage 100%, all tests passing.

@coveralls
Copy link

Coverage Status

Coverage decreased (-90.8%) to 9.231% when pulling 17c010d on izeau:master into 09001fe on markdalgleish:master.

@izeau
Copy link
Author

izeau commented Aug 28, 2017

There seems to have been an issue with the Node version used by Travis for its testing. I’ve updated it to Node 6 (LTS Boron).

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 8259585 on izeau:master into 09001fe on markdalgleish:master.

@izeau
Copy link
Author

izeau commented Aug 28, 2017

There’s probably an issue with the generated GZIP (looks like even if the source HTML / gunzip’d HTML are the exact same, some bits are different in the GZIP itself). For reference I’m on macOS 10.12.6 and using compression-webpack-plugin version 1.0.0.

Imho it’s not safe to diff the .html.gz file itself, are there any workaround in place in static-site-generator-webpack-plugin to justify having a fixture to test CompressionPlugin's integration?

Edit: reverted the index.html.gz change to use this repo's file

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 538f142 on izeau:master into 09001fe on markdalgleish:master.

@rafa-suagu
Copy link

Hi all, thanks for your amazing work! you can merge this pull request? Thanks!!

@AlasdairSwan
Copy link

Great work @izeau, any idea when this is going to merge?

@izeau
Copy link
Author

izeau commented Oct 19, 2017

Hey @rafa-aguilar and @AlasdairSwan, unfortunately I’m not an owner of this repository and therefore cannot merge this PR. Meanwhile, feel free to use my fork.

@heisian
Copy link

heisian commented Dec 1, 2017

@markdalgleish Anyone home?

@heisian
Copy link

heisian commented Dec 15, 2017

This PR I believe is not needed. The correct CommonsChunkPlugin configuration is needed. This is what works for me:

const plugins = [
  new webpack.optimize.CommonsChunkPlugin({
    name: 'vendor',
    chunks: ['vendor', 'webApp'],
    minChunks: (module) => {
      return module.context && module.context.includes('node_modules');
    },
  }),

  new webpack.optimize.CommonsChunkPlugin({
    name: 'manifest',
    chunks: ['vendor', 'webApp'],
    minChunks: Infinity,
  }),

  new StaticSiteGeneratorPlugin({
    entry: 'prerenderedApp',
    paths: [
      '/',
      '/about',
      '/faq',
      '/terms',
      '/login',
      '/privacy',
    ],
    locals: {
      ejs, // Pass in templating engine to static.js
      clientPath: config.paths.client(),
      contentSecurityPolicy,
    },
    globals: {
      window: {
        navigator: {},
        matchMedia: () => ({}),
        addEventListener: () => {
        },
        analytics: {
          page: () => {
          },
        },
      },
    },
 }),
];

This configuration lets me build chunked manifest, vendor, and webApp bundles, while at the same time generating a non-chunked prerenderedApp bundle that static-site-generator-webpack-plugin can use to output prerendered HTML.

Thus, both static and dynamic apps are generated in one compilation.

@grajagandev
Copy link

Would you be interested in software that automatically finds ReferenceErrors and TypeErrors, caused by corner cases like these? I am building Fuzz Stati0n to do that (free for OSS) - please take a look and consider signing up for our newsletter to keep in touch.

@heisian
Copy link

heisian commented Dec 19, 2017

Pretty sure your advertisement has nothing to do with this PR.

@izeau
Copy link
Author

izeau commented Dec 20, 2017

@heisian nice one! I can’t test it right now but I’ll give it a look. It’s been a while, I don’t really remember the configuration I used. Thanks!

@heisian
Copy link

heisian commented Dec 20, 2017

@izeau I spent a few days using your PR at first, but I was still getting call to undefined..., as I'm sure my setup differs from yours. So digging more I just kept building and changing configs.. couldn't really find any useful CommonsChunkConfigs that matched mine so I had to do some thinking to come up with what I put. I hope it helps your build.

BTW i have multiple entrypoints in my webpack config:

...
entry: {
    webApp: './index',
    prerenderedApp: './static',
    vendor: [
      'react',
      'react-burger-menu',
      'react-countup',
      'react-dom',
      'react-fa',
      'react-hammerjs',
      'react-helmet',
      'react-redux',
      'react-router-dom',
      'react-select',
      'react-text-mask',
      'redux',
      'redux-form',
      'redux-thunk',
    ],
  },
...

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.

None yet

7 participants