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

Use polka instead of express to serve the viewer #212

Closed
wants to merge 1 commit into from

Conversation

realityking
Copy link
Contributor

While the code is less elegant, this is a pretty big size reduction for anyone not using express as you can see in the number of dependencies that become dev dependencies. On the other hand, it's a relatively small size increase for those projects that do use polka.

Express is still the most popular web framework but I wouldn't assume that everyone is using it since there are so many great alternatives. Since bundle analyser has simple needs, something tiny like polka should do.

src/viewer.js Outdated

ejs.renderFile('views.ejs', data, ejsOptions, (err, str) => {
if (err) {
throw err;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't err be passed to the next route handler?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't a middleware but a handler for GET and thus doesn't have a next handler. I could output an error to the browser but I figured this be enough for the use case of bundle analyzer.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With Polka@next, all route handlers follow (req, res, err) signature, so you're no longer limited here.

Here's an alternative approach using the current polka@0.5 version

package.json Outdated
"filesize": "^3.6.1",
"gzip-size": "^5.0.0",
"lodash": "^4.17.10",
"mkdirp": "^0.5.1",
"opener": "^1.5.1",
"polka": "^0.5.1",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does it reduce the total size of dependencies? I would like to see the numbers.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@realityking
Copy link
Contributor Author

Fair question on the size. Using bundle phobia:

webpack-bundle-analyzer is currently at 6.05MB, express is 1.56 MB so a quarter of that.

polka is just 53kB and serve-static is 318kB.

So this PR would shave off a bit more than 1MB of the install size.

@th0r
Copy link
Collaborator

th0r commented Oct 4, 2018

For me node_modules sizes using npm ci --production are 10mb with express and 8.3mb with polka. Although it's a quite significant difference I would better wait for the polka to reach v1 and stabilize it's API before switching to it.

Thanks for the PR! I'll leave it opened as a reminder.

@realityking
Copy link
Contributor Author

@th0r I don't think polka will receive a 1.0 anytime soon. It's more likely one of those projects that (annoyingly) stays < 1.0 forever.

Since polka has a very small API and bundle analyzer makes only 3 calls to it, I think migrating to a new API would be easy.

We could pin to a fixed version if that helps. What do you think?

@lukeed
Copy link

lukeed commented Jul 4, 2020

Hi! FWIW polka@next (and all others publicly available with the next tag) are completely stable and have been for quite some time. They're already powering multiple high-traffic production applications.
The only reason 1.0 hasn't been released yet is because I'm finalizing new module packages and adding a documentation site. The new packages would focus on new alternatives for things like cookie handling, compression, etc. What's publicly available now is finalized 👍

package.json Outdated
"filesize": "^3.6.1",
"gzip-size": "^5.0.0",
"lodash": "^4.17.15",
"mkdirp": "^0.5.1",
"opener": "^1.5.1",
"polka": "^0.5.2",
"serve-static": "^1.14.1",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest sirv here instead for further size (and performance) improvements. Unfortunately serve-static is a significant portion of Express' size. You can find an example with Polka here

@realityking
Copy link
Contributor Author

I've rebased this PR to be mergeable again. And also adopted sirv as suggested by @lukeed.

I've did stick to polka@^0.5.2 instead of using the preview releases.

@lukeed It'd be great to a stable polka 1.0 :)

@realityking
Copy link
Contributor Author

realityking commented Nov 16, 2020

Comparison of node_modules sizes:

  • This branch: 39 packages weighing 3,9 MB
  • Master: 80 packages weighing 5,4 MB

Half the packages and 28% less bytes :)

TrySound added a commit to TrySound/webpack-bundle-analyzer that referenced this pull request Nov 28, 2020
Alternative to long-standing webpack-contrib#212

Express in this project does not add much value. Middleware api is not
necessary for 1.5 middlwares. We can try to use node server directly.

Serving static is left for express middleware. Can be replaced with
something smaller in another PR.
@realityking
Copy link
Contributor Author

Updated this in light of #397 being merged.

New comparison of node_modules:

  • This branch: 34 packages weighing 4,4 MB
  • master: 65 packages weighing 5,9 MB

Still ~half the packages and 20% less bytes.

(I have no idea why the size went up compared to the last time I ran the stats)

TrySound added a commit to TrySound/webpack-bundle-analyzer that referenced this pull request Nov 30, 2020
Alternative to long-standing webpack-contrib#212

Express in this project does not add much value. Middleware api is not
necessary for 1.5 middlwares. We can try to use node server directly.

Serving static is left for express middleware. Can be replaced with
something smaller in another PR.
TrySound added a commit to TrySound/webpack-bundle-analyzer that referenced this pull request Nov 30, 2020
Alternative to long-standing webpack-contrib#212

Express in this project does not add much value. Middleware api is not
necessary for 1.5 middlwares. We can try to use node server directly.

Serving static is left for express middleware. Can be replaced with
something smaller in another PR.
@valscion
Copy link
Member

valscion commented Dec 1, 2020

Thank you for the contributions! In the end, we went with #398 ☺️

@valscion valscion closed this Dec 1, 2020
@realityking
Copy link
Contributor Author

All good. Thank you :)

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

4 participants