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

Enable source maps in webpack chunking + bundling process #3793

Merged
merged 13 commits into from Mar 6, 2018

Conversation

Projects
None yet
8 participants
@ptomasroos
Copy link
Contributor

commented Feb 13, 2018

Its impossible to get good source maps at this moment in next.js
The reason for this is that the "manual" concat of sources happens outside the sourcemap generation which ends up resulting in, guess what! No source maps for app.js (which is quite crucial because it often exists of majority of the application)

➜  web git:(fix-850-source-maps) ✗ ls -l .next 
total 12552
-rw-r--r--  1 tomas  staff       36 Feb  9 09:43 BUILD_ID
-rw-r--r--  1 tomas  staff  1195881 Feb  9 09:43 app.js
-rw-r--r--  1 tomas  staff       54 Feb  9 09:43 build-stats.json
drwxr-xr-x  3 tomas  staff       96 Feb  9 09:43 bundles
-rw-r--r--  1 tomas  staff  1063099 Feb  9 09:43 commons.js
-rw-r--r--  1 tomas  staff  3728876 Feb  9 09:43 commons.js.map
drwxr-xr-x  4 tomas  staff      128 Feb  9 09:43 dist
-rw-r--r--  1 tomas  staff    29961 Feb  9 09:43 main.js
-rw-r--r--  1 tomas  staff   117438 Feb  9 09:43 main.js.map
-rw-r--r--  1 tomas  staff     3135 Feb  9 09:43 manifest.js
-rw-r--r--  1 tomas  staff    10791 Feb  9 09:43 manifest.js.map
-rw-r--r--  1 tomas  staff    99826 Feb  9 09:43 react.js
-rw-r--r--  1 tomas  staff   124422 Feb  9 09:43 react.js.map
-rw-r--r--  1 tomas  staff    25859 Feb  9 09:43 service-worker.js

This is sad of course!

This PR aims to fix this by removing the home written source concat and using CommonChunksPlugin to combine main, manifest (added to last defined common chunk, this is the bootstrap code of webpack), react and the commons stuff.

In order to use source-maps one needs to alter the webpack config through next.config and enable it with the following

      config.devtool = 'source-map';

      // Perform customizations to config existing plugins
      for (const options of config.plugins) {
        if (options['constructor']['name'] === 'UglifyJsPlugin') {
          options.options.sourceMap = true;
          break;
        }
      }

First set webpack's devtool to 'source-map' in order to get external maps.

And then we're using Uglify and there we need to find the plugin and override the default sourceMap: false from "user-land" config.

total 10424
-rw-r--r--  1 tomas  staff       36 Feb 13 21:05 BUILD_ID
-rw-r--r--  1 tomas  staff  1203400 Feb 13 21:05 app.js
-rw-r--r--  1 tomas  staff  4094606 Feb 13 21:05 app.js.map
-rw-r--r--  1 tomas  staff       54 Feb 13 21:05 build-stats.json
drwxr-xr-x  3 tomas  staff       96 Feb 13 21:05 bundles
drwxr-xr-x  4 tomas  staff      128 Feb 13 21:05 dist
-rw-r--r--  1 tomas  staff    25859 Feb 13 21:05 service-worker.js

ptomasroos added some commits Feb 13, 2018

@timneutkens timneutkens requested review from arunoda and timneutkens Feb 14, 2018

@@ -287,7 +282,7 @@ export default async function getBaseWebpackConfig (dir, {dev = false, isServer
return count >= totalPages * 0.5
}
}),
!isServer && new webpack.optimize.CommonsChunkPlugin({
dev && !isServer && new webpack.optimize.CommonsChunkPlugin({

This comment has been minimized.

Copy link
@timneutkens

timneutkens Feb 14, 2018

Member

This negates the react split bundle optimization with uglify. It might not be needed anymore because of the uglify optimizations we did though. cc @arunoda

This comment has been minimized.

Copy link
@arunoda

arunoda Feb 14, 2018

Contributor

@ptomasroos let's remove this as well.

This comment has been minimized.

Copy link
@ptomasroos

ptomasroos Feb 15, 2018

Author Contributor

Done

@arunoda
Copy link
Contributor

left a comment

Yep. I like this approach.
Have a look at my suggestions below.

!dev && new webpack.optimize.ModuleConcatenationPlugin(),
!isServer && new PagesPlugin(),
!isServer && new DynamicChunksPlugin(),
isServer && new NextJsSsrImportPlugin({ dir, dist: config.distDir }),
!isServer && new webpack.optimize.CommonsChunkPlugin({
dev && !isServer && new webpack.optimize.CommonsChunkPlugin({

This comment has been minimized.

Copy link
@arunoda

arunoda Feb 14, 2018

Contributor

Remove this completely. We are removing the react bundle too.
So, there's no need this in dev.

This comment has been minimized.

Copy link
@ptomasroos

ptomasroos Feb 15, 2018

Author Contributor

Done

@@ -287,7 +282,7 @@ export default async function getBaseWebpackConfig (dir, {dev = false, isServer
return count >= totalPages * 0.5
}
}),
!isServer && new webpack.optimize.CommonsChunkPlugin({
dev && !isServer && new webpack.optimize.CommonsChunkPlugin({

This comment has been minimized.

Copy link
@arunoda

arunoda Feb 14, 2018

Contributor

@ptomasroos let's remove this as well.

@@ -306,9 +301,34 @@ export default async function getBaseWebpackConfig (dir, {dev = false, isServer
return false
}
}),
!isServer && new webpack.optimize.CommonsChunkPlugin({
dev && !isServer && new webpack.optimize.CommonsChunkPlugin({

This comment has been minimized.

Copy link
@arunoda

arunoda Feb 14, 2018

Contributor

Let's keep this.
This helps in bundle rebuilding process.

@arunoda
Copy link
Contributor

left a comment

have a look at these suggestions too.
After that, we may've to few other changes.

I'll send a patch for that.

}),
!dev && !isServer && new webpack.optimize.CommonsChunkPlugin({
name: 'main.js',
filename: 'app.js',

This comment has been minimized.

Copy link
@arunoda

arunoda Feb 14, 2018

Contributor

We can keep this as main.js.
And in the server/document.js we can change to use main.js instead of app.js for both production and dev.

For dev you may need to use manifest.js before main.js.

This comment has been minimized.

Copy link
@ptomasroos

ptomasroos Feb 15, 2018

Author Contributor

Done

ptomasroos added some commits Feb 15, 2018

@ptomasroos

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2018

All right @arunoda I've fixed what you mentioned and also cleaned up the fix around late bundling regarding the build-stats

does it make sense to update the two failing tests or not since its not manual code in anyway anymore?

  describe('File locations', () => {
    it('should build the app within the given `dist` directory', () => {
      expect(existsSync(join(__dirname, '/../dist/main.js'))).toBeTruthy()
    })

    it('should not build the app within the default `.next` directory', () => {
      expect(existsSync(join(__dirname, '/../.next/main.js'))).toBeFalsy()
    })
  })

@ptomasroos

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2018

Ping @arunoda

@nilportugues

This comment has been minimized.

Copy link

commented Mar 2, 2018

Hey, can we get this merged ASAP?

Ping @arunoda

@juanramon

This comment has been minimized.

Copy link

commented Mar 2, 2018

Ping @arunoda

@timneutkens timneutkens referenced this pull request Mar 5, 2018

Closed

Production Sourcemaps on v5.0.0 #3934

1 of 1 task complete
@arunoda

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2018

@ptomasroos we were waiting because of our Webpack v4 migration. Seems like we need to wait a bit more for it.

Let me fix conflicts on this.

@timneutkens

This comment has been minimized.

Copy link
Member

commented Mar 5, 2018

@nilportugues we always thoroughly verify all PRs changing default behaviour. So sometimes it takes a while.

@juanramon it doesn't make sense to send ping twice in a hour. It won't make a maintainer merge the pull request any faster, and might even be viewed as aggressive by some people (not us), just a small advice 👍

🙏

@ptomasroos

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2018

Thanks @arunoda feel free to fix the conflicts

@arunoda

arunoda approved these changes Mar 6, 2018

@arunoda

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2018

@ptomasroos I've fixed the conflict and make sure tests are passing.
Have look at the changes.

@timneutkens could you do a final review and take this in.

@ptomasroos

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2018

Thanks @arunoda all looks good

@timneutkens
Copy link
Member

left a comment

Looks great, cleans up a lot of code too 😌

@timneutkens timneutkens merged commit 76582b8 into zeit:canary Mar 6, 2018

2 of 3 checks passed

coverage/coveralls Coverage pending from Coveralls.io
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@devcorpio

This comment has been minimized.

Copy link

commented Mar 7, 2018

Hi @timneutkens,

Only for know, how much time usually do you wait before merge canary branch into master? With this change will begin to appear the map of the file app.js?

Regards,

@timneutkens

This comment has been minimized.

Copy link
Member

commented Mar 7, 2018

We're currently finishing up for 5.1. No ETA yet, but this has been release as next@canary already 👍

@oliviertassinari

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2018

I'm happy to see the following diff 💯

.size-limit

  {
    "name": "The main bundle",
    "webpack": false,
-   "path": ".next/app.js",
+   "path": ".next/main.js",
    "limit": "178 KB"
  },```
@timneutkens

This comment has been minimized.

Copy link
Member

commented Mar 10, 2018

Amazing 😲

@devcorpio

This comment has been minimized.

Copy link

commented Mar 11, 2018

Then, from now, nextjs will be generate main.js(and main.js.map) instead of app.js?

That change will be helpful for my workmates and me, will help us using sentry reporting tool :)

@timneutkens

This comment has been minimized.

Copy link
Member

commented Mar 11, 2018

Yep @devcorpio given you configure next.config.js correctly 👍

@bryandowning

This comment has been minimized.

Copy link

commented Mar 15, 2018

Any chance this would have messed with @zeit/next-sass? My sass doesn't compile correctly after upgrading from next@5.0.1-canary.10 to next@5.0.1-canary.11. I'm having a hard time reproducing a simple example. It's weird because some of the styles get output correctly, but other component styles are omitted. This is happening in dev and production builds, although in production builds WAY more styles are omitted.

@timneutkens

This comment has been minimized.

Copy link
Member

commented Mar 15, 2018

@bryandowning most likely you're locked to an older version of next-css. Try removing the lockfiles, removing node_modules and reinstalling and upgrading next-sass. I'll do a new release soon that locks all versions of next-plugins, then this won't be happening again, because it'll just lock to the version specified.

@axsann axsann referenced this pull request Apr 10, 2018

Closed

Production Source Maps #1903

@lock lock bot locked as resolved and limited conversation to collaborators Mar 15, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.