-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Process inline styles and scripts #1456
Conversation
src/assets/HTMLAsset.js
Outdated
async postProcess(generated) { | ||
// Hacky way of filtering out the css hmr JS | ||
// Related: https://github.com/parcel-bundler/parcel/issues/1389 | ||
generated = generated.filter( |
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 line might cause issues, related to this issue: #1389
There is currently no way to tell in generated you don't want the hmr code for css assets.
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.
I think this won't work if HMR is actually enabled - the generated code won't be empty in that case.
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.
Also probably need to filter out source maps or they'll mess up the indexing below
src/assets/HTMLAsset.js
Outdated
if (node.content) { | ||
let value = node.content.join('').trim(); | ||
if (value) { | ||
parts.push({ |
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 is a bit too clever for my IQ 😅
This PR looks great! One thing I noticed is it doesn't handle inline I think it is handled in |
@stevengill Not sure what you mean, I got it to work in a new test I've added, could you create a small test-case that fails? |
my mistake @DeMoorJasper! Sorry about that. It is definiltey working. I tried it yesterday for a while and couldn't get it working. I think it was because I was using quotes in Thanks again! |
Alright, I've played around more with the branch and run into some issues. I've got a sample repo with a index.html that renders fine with master branch (minus the With this branch, the styles are all messed up. I haven't tracked down what in the html is causing this to happen with this branch yet. Please ignore the (text) in the html 😛. It is an internal email newsletter we sent out a while ago at work. I'll continue to play around with this and see if I can figure out what is going on. It would be really nice if I could actually add some console.logs to |
This is cool. How are we going to handle imports/requires inside the inline scripts/styles though? We'd need to run the packager to merge the modules together somehow, and then put the result back in the HTML... What happens right now when you put a |
Or maybe we don't try to inline |
@devongovett I originally made this to prevent any posthtml issues with js or css transformations. If you'd like this to handle requires as well it should probably inline the full bundle including the requires. Currently it works perfectly with requiring images and such in css which was one of the issues this PR resolves. For JS it currently just creates the JS bundle and throws an error, the bundle doesn't get added to html using the script tag |
src/assets/HTMLAsset.js
Outdated
@@ -62,6 +62,11 @@ const META = { | |||
] | |||
}; | |||
|
|||
const SCRIPT_TYPES = { | |||
'application/javascript': 'js', | |||
'application/json': 'json' |
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.
parcel compiles json to javascript currently, so this might actually break things. haven't tested.
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.
It does, and it does mess up the inlining, might have to look into that
src/assets/HTMLAsset.js
Outdated
async postProcess(generated) { | ||
// Hacky way of filtering out the css hmr JS | ||
// Related: https://github.com/parcel-bundler/parcel/issues/1389 | ||
generated = generated.filter( |
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.
I think this won't work if HMR is actually enabled - the generated code won't be empty in that case.
src/assets/HTMLAsset.js
Outdated
async postProcess(generated) { | ||
// Hacky way of filtering out the css hmr JS | ||
// Related: https://github.com/parcel-bundler/parcel/issues/1389 | ||
generated = generated.filter( |
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.
Also probably need to filter out source maps or they'll mess up the indexing below
src/assets/HTMLAsset.js
Outdated
parts.push({ | ||
type: | ||
node.tag === 'style' | ||
? 'css' |
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.
should we also support the type attribute on style elements as well to allow other languages like sass or stylus to be inlined in an HTML file? https://developer.mozilla.org/en-US/docs/Web/HTML/Element/style
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.
Sure, shouldn't be very hard
test/html.js
Outdated
@@ -277,11 +277,6 @@ describe('html', function() { | |||
) | |||
); | |||
|
|||
// minifyJson |
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.
why was this removed?
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.
Because htmlnano is no longer responsible for JSON minifying, although this caused the tests to not show that it actually changed json into JS so I've put it back
I do like the idea of this feature, we just have to make sure that it is not inconsistent with how the rest of parcel works. Extracting dependencies from inline CSS is obviously a good thing, but the fact that Maybe we start with this for now and design packagers in a way that's more compassable for Parcel 2 so that we can just use the CSS and JS packagers inside the HTML one for inline stuff. Then we can fully support |
@devongovett I`m glad to hear we have a plan to parcel2, anything else about the plan? |
Awesome, I’ll try to resolve those issues soonish Sent with GitHawk |
test/html.js
Outdated
@@ -638,4 +638,20 @@ describe('html', function() { | |||
|
|||
assert(html.includes('<style type="text/css">.index{color:#00f}</style>')); | |||
}); | |||
|
|||
it.only('should process inline non-js scripts', async function() { |
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.
Just a heads up, you've left an only
call here
Should be working in most cases now. If anyone would like to test this, please do. |
b37420f
to
bc2a891
Compare
Simplified slightly so that we actually pass through the original nodes to the pipeline so we can more easily replace their contents in postProcess without hackiness to detect things like css hmr. Also added some temporary handling for |
This PR changes the behaviour of inline scripts and styles in html to be processed by Parcel, similar to how we process Vue Assets.
Adds the ability to use files in css and modern syntax in JS.
Also removes the need for any minification for JS or CSS from htmlnano
Closes #541 Closes #1297 Closes #1231 Closes #1491