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

Fixed: Usage with CommonsChunks module #2

Closed
arunoda opened this issue Jul 2, 2017 · 8 comments
Closed

Fixed: Usage with CommonsChunks module #2

arunoda opened this issue Jul 2, 2017 · 8 comments

Comments

@arunoda
Copy link

arunoda commented Jul 2, 2017

Hey, this looks like pretty great.

I just started trying to integrate this with Next.js. (We've a pretty complex webpack config)
Specially we use the "common chunks plugin".

When I use this as mentioned in the recommend way, I got react and react-dom in the vendor.js. But it's also in the commons.jsas well.

This is my code: vercel/next.js#2433

Is there anything else I've to do here.

@asfktz
Copy link
Owner

asfktz commented Jul 2, 2017

Hi, @arunoda,
I would love to help you!

I believe it's related to the context.
It must be set correctly. Otherwise, you'll get the same modules in both the DLL and the regular bundles.

This is one of the things that brought me a lot of headaches when I tried to figure out how the DLLPlugin works.

You should set the context to the root of the project.

For example:
If that is your project structure:

  • config/
    • webpack.config.js
  • src/
    • index.js
  • package.json

You should set the context like so:

module.exports = {
  entry: './src/index.js',

  ...

  plugins: [
    new AutoDllPlugin({
      context: path.join(__dirname, '..'),
      entry: {
        vendor: [
          'react',
          'react-dom'
        ]
      }
    })
  ]
};

I see that your context in your config is set to console.dir()

I plan to make it more clear in the readme and also warn the user when a module exists in both the DLL and the main bundles.

I see that the commons.js created by the CommonsChunkPlugin.
I am not familiar with this CommonsChunkPlugin yet.
I would like test it to make sure the that using them both is not the cause of the problem.

also, I see that you use the inject option, but you reference the DLL bundle yourself in server/document.js

getScripts () {
    const { dev } = this.context._documentProps
    if (dev) {
      return [
        this.getChunkScript('manifest.js'),
        this.getChunkScript('commons.js'),
        this.getChunkScript('vendor.js'),
        this.getChunkScript('main.js')
      ]
    }

    ...
}

The inject: true option is used to inject the script tags into the HTML for you,
but only when the HtmlPlugin is used, because AutoDLL needs to connect to the hooks it exposes.
Otherwise this is no-op.

This is why the recommended way also includes the HtmlPlugin
I really need to make it more clear in the readme.

Please let me know if you need any further help (:

Asaf

@arunoda
Copy link
Author

arunoda commented Jul 3, 2017

@asfktz Thanks for the help.
We don't have webpack config file, but I assume the context we set is correct.

Thanks for the mention about the inject flag.
I'll try more.

@asfktz
Copy link
Owner

asfktz commented Jul 3, 2017

I'll try to run your fork locally, to see if I can help you set it up.
Hope to get to it later tonight.

@arunoda
Copy link
Author

arunoda commented Jul 3, 2017

@asfktz that'd be amazing.
Thanks.

@asfktz
Copy link
Owner

asfktz commented Jul 5, 2017

Turns out It was the context after all, but in a different way than I expected.

Your bundles look like that:

{
  'main.js': [ '/Users/asafkatz/dev/next.js/dist/client/next.js' ],
  'bundles/pages/index.js': [ './pages/index.js?entry' ],
  'bundles/pages/other.js': [ './pages/other.js?entry' ],
  'bundles/pages/_error.js': [ '/Users/asafkatz/dev/next.js/dist/pages/_error.js?entry' ],
  'bundles/pages/_document.js': [ '/Users/asafkatz/dev/next.js/dist/pages/_document.js?entry' ]
}

(note that next.js is a symlink, but it should work as a regular dependency too)

We got two different contexts here.

For the pages bundles, it was the user's project root as expected (dir in your case),
but for main.js - it's a different one.

I was surprised to discover that in the case of /Users/asafkatz/dev/next.js/dist/client/next.js',
the context is /Users/asafkatz/dev/next.js

I never saw an absolute path as an entry module before, It's good to be aware that such use case exists.

The solution is to set the context like so:

new AutoDllPlugin({
      context: join(__dirname, '../../..'),  // '/Users/asafkatz/dev/next.js'
      filename: '[name].js',
      path: './',
      entry: {
        vendor: [
          'react',
          'react-dom'
        ]
      }
}),

Theatrically, you should be able to use 2 instances of AutoDllPlugin at once, one for each context.

[
  new AutoDllPlugin({
        context: join(__dirname, '../../..'),  // '/Users/asafkatz/dev/next.js'
        filename: '[name].js',
        path: './',
        entry: {
          vendor: [
            'react',
            'react-dom'
          ]
        }
  }),

  new AutoDllPlugin({
        context: dir,  // The user's root project
        filename: '[name].js',
        path: './',
        entry: {
          'user-vendors': [
            'cowsay-browser'
          ]
        }
  })
]

But it will not work in the current version because of the way the cache is handled.
It will be fixed soon.

Also, I believe #6 may affect you.
It's the next thing on my list.

Really enjoyed to dive into the internals of next.js, it's beautiful!
And I also learned a few things along the way (:

Please let me know if you need any further help.
Asaf.

@arunoda
Copy link
Author

arunoda commented Jul 5, 2017

Thanks for @asfktz for digging into this.
Ping me when you've the cache fix.

@asfktz asfktz mentioned this issue Jul 5, 2017
@asfktz
Copy link
Owner

asfktz commented Jul 11, 2017

Hi, @arunoda!
How are you?

I wanted to let you know that the cache problem has fixed
And a few more bugs along the way.

I forked your fork to test it with the new version of the plugin:
https://github.com/asfktz/next.js/tree/with-autodll

You'll see that there are now 2 AutoDll instances, one for next.js and one for the user.
For the user DLL, I hard coded the dependencies from the with-redux, just for simplicity sake.

If you'll inspect the source of commons.js and main.js you'll see that they no longer contains the modules specified in the DLL entries.

I am curious to know how you intend to use it.

It's important for me to learn from the use cases so the plugin could evolve in the right direction.
So if you feel that it should work differently then the way it works now - I'd like to know.

Please let me know if you need any further help.
I'd be happy to help you.

Asaf.

@asfktz asfktz changed the title Usage with CommonsChunks module Fixed: Usage with CommonsChunks module Jul 14, 2017
@arunoda
Copy link
Author

arunoda commented Jul 15, 2017

@asfktz Nice. I'll play along with this.

@asfktz asfktz closed this as completed Aug 16, 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

No branches or pull requests

2 participants