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

A way to stop loading the same module multiple times. #253

Closed
arunoda opened this issue Nov 14, 2016 · 25 comments
Closed

A way to stop loading the same module multiple times. #253

arunoda opened this issue Nov 14, 2016 · 25 comments

Comments

@arunoda
Copy link
Contributor

arunoda commented Nov 14, 2016

Have a look at the following two pages:

pages/index.js

import React from 'react'
import Counter from '../components/Counter'
import Link from 'next/link'

console.log("Home", Counter());

export default () => (
  <div>
    <p>Home</p>
    <Link href="/about" />
  </div>
)

pages/about.js

import React from 'react'
import Counter from '../components/Counter'

console.log("About", Counter());

export default () => (
  <div>
    <p>About</p>
  </div>
)

And here's the Counter component

components/Counter.js

console.log('Counter');

let counter = 0;

export default () => {
  return ++counter;
}

Here's the situation:

  • Visit "/" - Then the Counter module gets loads for the first time
  • Then I visit "/about" by clicking the link on the home page - Then the Counter module loads again.

This gives us two issues:

  • Loading common code more than once.
  • Our app might not work properly. (Here our counter is not gonna work properly as there are two such modules)

Possible solution

  1. Allow the developer to define a set of modules as common modules.
  2. Then bundle them into a separate output and load with every request.
  3. We also need to add them to externals in the webpack config.
@sedubois
Copy link
Contributor

👍 This thing of having something being maintained across pages is IMHO quite needed for performance and functionality, also for Redux/Apollo, page templates, CSS transitions, this kind of stuff.

See #50, #75, #88, #106, #203.

Does #25 solve this?

@arunoda
Copy link
Contributor Author

arunoda commented Nov 14, 2016

@sedubois Thanks for adding references.

I don't think #25 solves this as we need to change the webpack config as well.
We need to load a that file with every page too.

@rauchg
Copy link
Member

rauchg commented Nov 16, 2016

Yeah this one is interesting. I would definitely expect the module to be initialized only once.

@rauchg
Copy link
Member

rauchg commented Nov 16, 2016

In other words, I don't think we need a way to prevent it, we just need to make it work as expected.

@arunoda
Copy link
Contributor Author

arunoda commented Nov 16, 2016

FYI:

This is not happening with normal Webpack based code splitting. Still, they they load duplicate modules but only load the module once.
They do that by moving all the webpack related modules into common file. I hope they do it with the CommonChunksPlugin.

May be it's time we should try to use Webpack2 and System.import internally.

@nkzawa what are blocks for us to use that.

Otherwise, we may have to write a lot of stuff.
(May be we could reuse some of the stuff webpack comes with)

@nkzawa
Copy link
Contributor

nkzawa commented Nov 16, 2016

webpack2 is still beta, so I worry it might be unstable and can be incompatible with external plugins. I think there is no feature we needed on webpack2, or does it help to solve the issue?

About CommonChunksPlugin, as I understand it can cause to load unneeded modules to pages.
Currently, pages are completely isolated which is really a nice feature of next.js IMO.

@rauchg
Copy link
Member

rauchg commented Nov 16, 2016

I'm +1 on moving to webpack2, since it also gives us tree shaking

@arunoda
Copy link
Contributor Author

arunoda commented Nov 16, 2016

I assume Webpack 2 is ready. They just wanted to just ship the new website.
And they did it: https://webpack.js.org/

I assume moving to Webpack 2 let us to worry about important stuff while Webpack do what best it is.

When it's time to configure, we can simply ask them to refer website docs. APIs.

For an example, optimitizing code splitting is pretty important and it's hard to guess that in the framework level. See: https://webpack.js.org/plugins/commons-chunk-plugin/#move-common-modules-into-the-parent-chunk

@arunoda
Copy link
Contributor Author

arunoda commented Nov 16, 2016

Additionally, we should do it in a way where the developer doesn't need to worry about System.import API. We could do it in the Link component.

@rauchg
Copy link
Member

rauchg commented Nov 16, 2016

Yeah. This is connection to a big improvement we can make for route code evaluation.
We already make a JSON query to the route URL so that it returns the top-level component's code.

So, let's say today you have:

/
/docs
/code
/api

Let's say both /api and /code depend on Code Mirror. Right now we're duplicating code mirror in the returned JS string.

Ideally, the JSON response would look like this for /api.json let's say:

{
  "name": "api",
  "bundles": [
     "x.js",
     "z.js"
  ]
}

z.js would contain codemirror, and the /code response would include it:

{
  "name": "api",
  "bundles": [
     "y.js",
     "z.js"
  ]
}

In addition, ideally we load the scripts in parallel (injecting them as <script> instead of evaluating). When they all load successfully (Promise.all), we finish the <Link> transition.

@nkzawa
Copy link
Contributor

nkzawa commented Nov 17, 2016

I think we can create another issue for moving to webpack2.

@rauchg I think that would be really slow in some cases if it's not HTTP/2.

A solution is that we fetch next page as a chunk like we do now, but reuse modules within the chunk if there are caches on client. It's not ideal but there is no downside compared to the current situation.

@rauchg
Copy link
Member

rauchg commented Nov 17, 2016

Yeah, another option is to advertise what bundles the client already has in the request, but that can add up to be huge.

We have to balance the cost / benefit of the extra roundtrip. If most people (like us) will deploy using pre-fetching, it's not bad.

@Nishchit14
Copy link

Nishchit14 commented Jan 16, 2018

@arunoda I couldn't see any solution, This issue has been fixed? I have seen many issues related to it (Top Level Component/ Common Layout), some of them are open and some of them are closed due to a long talk.

please share if it has been fixed and merged in any release, thanks for your kind work :)

@djskinner
Copy link

I'd be interested to hear updates on this. Discovering that multiple 'instances' of the same modules can exist is completely different to the expected behaviour of nearly every JavaScript module implementation. Seemingly it makes it impossible to create a singleton for example.

@Nishchit14
Copy link

@djskinner It would have the END here
#3552

@joaovieira
Copy link

joaovieira commented Jun 13, 2018

How did this get fixed? If my _app.jsx imports a module also used by another page.jsx it'll still duplicate and initialize the same module twice. E.g.:

// api.js
let token;

export const setToken (t) => {
  token = t;
}

export default const api = () => {
  console.log('Sending request with token: ', token);
}
// _app.jsx
import { setToken } from './api.js';

export default MyApp extends App {
  static getInitialProps ({ ctx }) {
    setToken(ctx.req? ctx.req.cookies['token'] : Cookies.get('token'));
    ....
  }

  ...
}
// page.jsx
import api from './api';

class Page extends Component {
  static getInitialProps () {
    api();  // should log correct auth token
  }
}

However, at least in development on the server, I get the auth module duplicated in both _app.js and page.js bundles, which will result in the wrong result.

  1. Is the _app.jsx the best place to run "initialization" code using the server req?
  2. how to ensure api.js is a singleton - only defined/executed once

@timneutkens
Copy link
Member

This has been fixed. Will be available in the upcoming canary release.

@onufriievkyrylo
Copy link

@timneutkens I just faced the same problem as @joaovieira.
As I understand it is not expected behavior. When it could be fixed and what behavior I can expect after?
Also, could you advice a temporary solution for this problem?

@timneutkens
Copy link
Member

timneutkens commented Jun 14, 2018

The fix is on next@canary, and it will bundle modules used in _app only into _app, as _app.js is used in all pages.

@onufriievkyrylo
Copy link

onufriievkyrylo commented Jun 15, 2018

@timneutkens, so if I import the same module to the _app and page component, in the page component it will be reinitialize. Am I right?
Actually I'm just tried with the latest canary. As I understand on the client side when I initialize module and use it on the pages module doesn't reinitialize. But on the server side when I'm import module on a page what was already importer by _app it's reinitialize.

@gh0stonio
Copy link

@timneutkens thx a lot for the fix, i confirm that it's fix my similar issue, but since i upgrade my next version from 6.1.1 to 6.1.1-canary.4 the HMR seems to not working anymore, are you aware of this ?

@gh0stonio
Copy link

gh0stonio commented Sep 17, 2018

Hi @timneutkens , the fix doesn't seems to be included in the 6.1.2, do you know when it will be merge in a non-canary version ?
I want to stop using a canary version in prod :)

Thanks a lot

@timneutkens
Copy link
Member

When next 7 comes out.

@gh0stonio
Copy link

Thanks a lot @timneutkens for your quick reply.
I will still use this canary version then until the next 7 release ;)

ijjk pushed a commit to ijjk/next.js that referenced this issue Apr 13, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Sep 17, 2019
@ijjk
Copy link
Member

ijjk commented Oct 1, 2019

Stats from current PR

Default Server Mode
General
zeit/next.js canary zeit/next.js canary Change
buildDuration 14.8s 14.5s -375ms
nodeModulesSize 48.2 MB 48.2 MB
Client Bundles (main, webpack, commons)
zeit/next.js canary zeit/next.js canary Change
main-HASH.js 18.1 kB 18.1 kB
main-HASH.js gzip 6.6 kB 6.6 kB
webpack-HASH.js 1.53 kB 1.53 kB
webpack-HASH.js gzip 746 B 746 B
4952ddcd88e7..2b8407376.js 21.9 kB 21.9 kB
4952ddcd88e7..7376.js gzip 7.81 kB 7.81 kB
de003c3a9d30..2ca0edb75.js 43.2 kB 43.2 kB
de003c3a9d30..db75.js gzip 15.5 kB 15.5 kB
framework.5b..dbaff70d3.js 125 kB 125 kB
framework.5b..70d3.js gzip 39.4 kB 39.4 kB
Overall change 210 kB 210 kB
Client Bundles (main, webpack, commons) Modern
zeit/next.js canary zeit/next.js canary Change
main-HASH.module.js 16.4 kB 16.4 kB
main-HASH.module.js gzip 6.35 kB 6.35 kB
webpack-HASH.module.js 1.53 kB 1.53 kB
webpack-HASH..dule.js gzip 746 B 746 B
de003c3a9d30..c0.module.js 45.6 kB 45.6 kB
de003c3a9d30..dule.js gzip 16.5 kB 16.5 kB
framework.5b..d3.module.js 125 kB 125 kB
framework.5b..dule.js gzip 39.4 kB 39.4 kB
Overall change 189 kB 189 kB
Client Pages
zeit/next.js canary zeit/next.js canary Change
_app.js 1.81 kB 1.81 kB
_app.js gzip 873 B 873 B
_error.js 12 kB 12 kB
_error.js gzip 4.73 kB 4.73 kB
hooks.js 12.7 kB 12.7 kB
hooks.js gzip 4.79 kB 4.79 kB
index.js 318 B 318 B
index.js gzip 222 B 222 B
link.js 8.14 kB 8.14 kB
link.js gzip 3.5 kB 3.5 kB
routerDirect.js 408 B 408 B
routerDirect.js gzip 281 B 281 B
withRouter.js 419 B 419 B
withRouter.js gzip 280 B 280 B
Overall change 35.8 kB 35.8 kB
Client Pages Modern
zeit/next.js canary zeit/next.js canary Change
_app.module.js 1.7 kB 1.7 kB
_app.module.js gzip 832 B 832 B
_error.module.js 23.3 kB 23.3 kB
_error.module.js gzip 8.59 kB 8.59 kB
hooks.module.js 1.52 kB 1.52 kB
hooks.module.js gzip 793 B 793 B
index.module.js 294 B 294 B
index.module.js gzip 223 B 223 B
link.module.js 8.53 kB 8.53 kB
link.module.js gzip 3.68 kB 3.68 kB
routerDirect.module.js 394 B 394 B
routerDirect..dule.js gzip 281 B 281 B
withRouter.module.js 404 B 404 B
withRouter.m..dule.js gzip 278 B 278 B
Overall change 36.1 kB 36.1 kB
Client Build Manifests
zeit/next.js canary zeit/next.js canary Change
_buildManifest.js 81 B 81 B
_buildManifest.js gzip 61 B 61 B
_buildManifest.module.js 81 B 81 B
_buildManife..dule.js gzip 61 B 61 B
Overall change 162 B 162 B
Rendered Page Sizes
zeit/next.js canary zeit/next.js canary Change
index.html 3.62 kB 3.62 kB
index.html gzip 946 B 946 B
link.html 3.66 kB 3.66 kB
link.html gzip 954 B 954 B
withRouter.html 3.67 kB 3.67 kB
withRouter.html gzip 941 B 941 B
Overall change 10.9 kB 10.9 kB

Serverless Mode
General
zeit/next.js canary zeit/next.js canary Change
buildDuration 15s 15.7s ⚠️ +686ms
nodeModulesSize 48.2 MB 48.2 MB
Client Bundles (main, webpack, commons)
zeit/next.js canary zeit/next.js canary Change
main-HASH.js 18.1 kB 18.1 kB
main-HASH.js gzip 6.6 kB 6.6 kB
webpack-HASH.js 1.53 kB 1.53 kB
webpack-HASH.js gzip 746 B 746 B
4952ddcd88e7..2b8407376.js 21.9 kB 21.9 kB
4952ddcd88e7..7376.js gzip 7.81 kB 7.81 kB
de003c3a9d30..2ca0edb75.js 43.2 kB 43.2 kB
de003c3a9d30..db75.js gzip 15.5 kB 15.5 kB
framework.5b..dbaff70d3.js 125 kB 125 kB
framework.5b..70d3.js gzip 39.4 kB 39.4 kB
Overall change 210 kB 210 kB
Client Bundles (main, webpack, commons) Modern
zeit/next.js canary zeit/next.js canary Change
main-HASH.module.js 16.4 kB 16.4 kB
main-HASH.module.js gzip 6.35 kB 6.35 kB
webpack-HASH.module.js 1.53 kB 1.53 kB
webpack-HASH..dule.js gzip 746 B 746 B
de003c3a9d30..c0.module.js 45.6 kB 45.6 kB
de003c3a9d30..dule.js gzip 16.5 kB 16.5 kB
framework.5b..d3.module.js 125 kB 125 kB
framework.5b..dule.js gzip 39.4 kB 39.4 kB
Overall change 189 kB 189 kB
Client Pages
zeit/next.js canary zeit/next.js canary Change
_app.js 1.81 kB 1.81 kB
_app.js gzip 873 B 873 B
_error.js 12 kB 12 kB
_error.js gzip 4.73 kB 4.73 kB
hooks.js 12.7 kB 12.7 kB
hooks.js gzip 4.79 kB 4.79 kB
index.js 318 B 318 B
index.js gzip 222 B 222 B
link.js 8.14 kB 8.14 kB
link.js gzip 3.5 kB 3.5 kB
routerDirect.js 408 B 408 B
routerDirect.js gzip 281 B 281 B
withRouter.js 419 B 419 B
withRouter.js gzip 280 B 280 B
Overall change 35.8 kB 35.8 kB
Client Pages Modern
zeit/next.js canary zeit/next.js canary Change
_app.module.js 1.7 kB 1.7 kB
_app.module.js gzip 832 B 832 B
_error.module.js 23.3 kB 23.3 kB
_error.module.js gzip 8.59 kB 8.59 kB
hooks.module.js 1.52 kB 1.52 kB
hooks.module.js gzip 793 B 793 B
index.module.js 294 B 294 B
index.module.js gzip 223 B 223 B
link.module.js 8.53 kB 8.53 kB
link.module.js gzip 3.68 kB 3.68 kB
routerDirect.module.js 394 B 394 B
routerDirect..dule.js gzip 281 B 281 B
withRouter.module.js 404 B 404 B
withRouter.m..dule.js gzip 278 B 278 B
Overall change 36.1 kB 36.1 kB
Client Build Manifests
zeit/next.js canary zeit/next.js canary Change
_buildManifest.js 81 B 81 B
_buildManifest.js gzip 61 B 61 B
_buildManifest.module.js 81 B 81 B
_buildManife..dule.js gzip 61 B 61 B
Overall change 162 B 162 B
Serverless bundles
zeit/next.js canary zeit/next.js canary Change
_error.js 253 kB 253 kB
_error.js gzip 67.9 kB 67.9 kB
hooks.html 3.75 kB 3.75 kB
hooks.html gzip 979 B 979 B
index.js 254 kB 254 kB
index.js gzip 68.2 kB 68.2 kB
link.js 261 kB 261 kB
link.js gzip 70.2 kB 70.2 kB
routerDirect.js 255 kB 255 kB
routerDirect.js gzip 68.3 kB 68.3 kB
withRouter.js 255 kB 255 kB
withRouter.js gzip 68.3 kB 68.3 kB
Overall change 1.28 MB 1.28 MB

Commit: b5da739

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests