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

Replace express with builtin node server #398

Merged
merged 5 commits into from Dec 1, 2020

Conversation

TrySound
Copy link
Contributor

@TrySound TrySound commented Nov 28, 2020

Alternative to long-standing #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.

Thanks to @realityking for inspiration

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.
src/viewer.js Outdated
@@ -13,6 +12,7 @@ const {open} = require('./utils');
const {renderViewer} = require('./template');

const projectRoot = path.resolve(__dirname, '..');
const viewsRoot = path.join(projectRoot, 'views');
Copy link
Contributor

@realityking realityking Nov 30, 2020

Choose a reason for hiding this comment

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

This line is not needed (merge artefact)

package.json Outdated
"filesize": "^6.1.0",
"gzip-size": "^6.0.0",
"lodash": "^4.17.20",
"opener": "^1.5.2",
"serve-static": "^1.14.1",
Copy link
Member

@valscion valscion Nov 30, 2020

Choose a reason for hiding this comment

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

Seems like as per this comment:

#212 (comment)

serve-static is a big reason why express is so large so if polka would work for our use case, it's much smaller than using serve-static.

Copy link
Member

@valscion valscion Nov 30, 2020

Choose a reason for hiding this comment

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

Oh, that other PR uses sirv for the static serving?

Copy link
Contributor Author

@TrySound TrySound Nov 30, 2020

Choose a reason for hiding this comment

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

Well, if you are ok to introduce sirv I will use it. I just remember there was a doubt to introduce polka packages.

Copy link
Member

@valscion valscion Nov 30, 2020

Choose a reason for hiding this comment

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

Oh, I don't really know what opinion @th0r has 😅 — sorry for being unclear. If things work then the size differences this provides might be useful?

As long as the server is still able to show new results when a bundle is changed like was the purpose behind introducing this feature back in #74 then I would be OK with this change.

I think the supported node versions are good enough, too, and we don't accidentally regress from supporting node v10.13.0

Copy link
Contributor Author

@TrySound TrySound Nov 30, 2020

Choose a reason for hiding this comment

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

To support watch I passed "dev" flag which prevents caching.
I guess the size will be almost the same but without more polka dependencies.
#212 (comment)

Copy link
Member

@valscion valscion Nov 30, 2020

Choose a reason for hiding this comment

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

The serve-static is still in dependencies so I don't know how to figure out the size difference yet — some numbers on how this potentially affects the size of node_modules would be nice.

Copy link
Contributor Author

@TrySound TrySound Nov 30, 2020

Choose a reason for hiding this comment

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

Copy link
Collaborator

@th0r th0r Dec 1, 2020

Choose a reason for hiding this comment

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

Why do we need serve-static if you decided to use sirv instead?

package.json Outdated
"filesize": "^6.1.0",
"gzip-size": "^6.0.0",
"lodash": "^4.17.20",
"opener": "^1.5.2",
"serve-static": "^1.14.1",
Copy link
Collaborator

@th0r th0r Dec 1, 2020

Choose a reason for hiding this comment

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

Why do we need serve-static if you decided to use sirv instead?

src/viewer.js Outdated Show resolved Hide resolved
src/viewer.js Show resolved Hide resolved
th0r
th0r approved these changes Dec 1, 2020
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