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

Add AOT gzip content-encoding support for main build files. #565

Merged
merged 3 commits into from
Dec 29, 2016

Conversation

arunoda
Copy link
Contributor

@arunoda arunoda commented Dec 29, 2016

Related to #555

Currently, we only do this for main.js and commons.js only.
We can do this for individual .json pages as well.

For that, we may need to change a bit of how we save pages and so on.
I think we could handle it in a different PR.

Currently we only do this for
main.js and commons.js only.
}

res.setHeader('Content-Encoding', 'gzip')
return await this.serveStatic(req, res, gzipPath)
Copy link
Member

Choose a reason for hiding this comment

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

return await is not needed. can just be return

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@rauchg
Copy link
Member

rauchg commented Dec 29, 2016

Would it be easy to write a test?

@rauchg
Copy link
Member

rauchg commented Dec 29, 2016

Adjusted the issue to divide the task in two parts per your recommendation @arunoda

@arunoda
Copy link
Contributor Author

arunoda commented Dec 29, 2016

@rauchg Yep.
I not sure about to have a test case since we don't do a production build right now.
May be we need to have a different test setup, targeting individual examples.

const nextDir = path.resolve(dir, '.next')

await gzip(path.resolve(nextDir, 'commons.js'))
await gzip(path.resolve(nextDir, 'main.js'))
Copy link
Member

Choose a reason for hiding this comment

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

await Promise.all could have a perf advantage here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. Parallelism.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@arunoda arunoda changed the title Add AOT gzip content-encoding support Add AOT gzip content-encoding support for main build files. Dec 29, 2016
@rauchg rauchg mentioned this pull request Dec 29, 2016
3 tasks
@rauchg rauchg merged commit 29c2267 into vercel:master Dec 29, 2016
@arunoda arunoda deleted the gzip branch December 30, 2016 10:10
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants