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
Exclude paths from html minification #200 #292
Exclude paths from html minification #200 #292
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Thanks, I saw that CI failed in test stage. I cannot reproduce the error in local. Do you know the possible reason? |
index.js
Outdated
@@ -210,6 +215,8 @@ function fastifyView (fastify, opts, next) { | |||
|
|||
function readCallback (that, page, data) { | |||
return function _readCallback (err, html) { | |||
const requestedPath = that?.request?.context?.config?.url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This syntax is not supported by the actual node10 CI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that request.context.config
should always exist
https://github.com/fastify/fastify/blob/70749bcc278ae9c4d1487ffc1bdecd67fb8bc14e/lib/fourOhFour.js#L127
https://github.com/fastify/fastify/blob/70749bcc278ae9c4d1487ffc1bdecd67fb8bc14e/lib/route.js#L226-L230
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for feedback. I removed optional chaining. Removing check on request existence causes errors on tests execution, in order to avoid these I added manual check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far it seems good to me :)
Just a little question/nit: art-template
and handlebars
have not been tested with this new feature, is there a particular reason to this ?
Thanks. I have not included tests for art-template and handlebars because art-template currently has not tests for minification (I thought it's not implemented) and handlebars has only tests without routing. |
Checklist
npm run test
andnpm run benchmark
and the Code of conduct