search

gvergnaud / nextjs-dynamic-routes

[Deprecated] Super simple way to create dynamic routes with Next.js
MIT License
140 stars 7 forks source link

Fix bundle size #18

Closed lydell closed 5 years ago

lydell commented 5 years ago

I made a mistake in #12 when saying this:

using the standard Node.js module 'url'. Next.js already uses it itself, so requireing it does not blow up the bundle size or anything (verified using next-bundle-analyzer).

I forgot to run npm run build in this repo when testing the new bundle size, so my "before" and "after" results were identical! Doh!

As you can see at bundlephobia, #12 did bump up the size of the package quite a lot (14 kB to 24 kB).

This happens because dist/ is compiled using webpack, pulling all dependencies into the bundle.

This commit fixes the problem by only using babel to compile dist/, leaving all require calls intact, letting Next.js' build take care of resolving and bundling them. This lets Next.js share the same 'url' module both for itself and for this package.

Size before this PR: 26.2 kB ![screenshot from 2019-01-25 08-09-54](https://user-images.githubusercontent.com/2142817/51731192-815d8700-207a-11e9-8449-483a669f8084.png)
Size after this PR: 12.8 kB ![screenshot from 2019-01-25 08-15-58](https://user-images.githubusercontent.com/2142817/51731239-ab16ae00-207a-11e9-9716-cb0c7f385e5e.png) Also, "loose mode" was turned on in .babelrc since it saves 1.5 kB and we don't use any features that require the extra spec compliance anyway.
gvergnaud commented 5 years ago

I don't think it's going to work. This code is supposed to run in the browser as well as on the server, and the 'url' package won't be available in the browser window. I think we should just revert your change from #12 and add the the missing features to parseUrl. Could you do that instead of changing the build system? I should have thought about it, my bad.

lydell commented 5 years ago

It does work, I’ve tried it!

gvergnaud commented 5 years ago

I fixed the issue, actually it was just because of the config of webpack-node-externals.

Here is the commit: https://github.com/gvergnaud/nextjs-dynamic-routes/commit/c92f1ae7692a9a5caf7fc0e75c8cee6fa593652b

You can see the result here: https://bundlephobia.com/result?p=nextjs-dynamic-routes@2.5.1

I'm closing your PR, because I'd like to keep webpack for now.

Thanks!

lydell commented 5 years ago

Nice! Just curious – is there are reason you want to keep webpack? Not using it saves another 0.5kB :)

And both ways result in require("url") in the built code.

https://unpkg.com/nextjs-dynamic-routes@2.5.1/dist/index.js

gvergnaud commented 5 years ago

It's just that the less diff in a PR/commit === the less potential bugs added :)