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

Process inline styles and scripts #1456

Merged
merged 15 commits into from Jul 21, 2018
Merged

Conversation

DeMoorJasper
Copy link
Member

@DeMoorJasper DeMoorJasper commented May 29, 2018

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

async postProcess(generated) {
// Hacky way of filtering out the css hmr JS
// Related: https://github.com/parcel-bundler/parcel/issues/1389
generated = generated.filter(
Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member

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

if (node.content) {
let value = node.content.join('').trim();
if (value) {
parts.push({
Copy link
Contributor

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 😅

@stevengill
Copy link

stevengill commented Jun 14, 2018

This PR looks great!

One thing I noticed is it doesn't handle inline background-image: url(./someImg.png). If i have that code in a css file, it works fine (realizes someImg.png is a dependency, hashes it and moves it over to dist, updates references to it)

I think it is handled in SASSAsset at https://github.com/parcel-bundler/parcel/blob/master/src/assets/SASSAsset.js#L41-L46 and CSSAsset at https://github.com/parcel-bundler/parcel/blob/master/src/assets/CSSAsset.js#L61-L77

@DeMoorJasper
Copy link
Member Author

@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?

@stevengill
Copy link

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 url("./SomeImg.png");

Thanks again!

@stevengill
Copy link

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 background-image:url('./someImg.png').

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 HTMLAsset so I could debug this. My logs never show up though (parcel-bundler is yarn linked)

@devongovett
Copy link
Member

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 require() call in your inline script?

@devongovett
Copy link
Member

Or maybe we don't try to inline require at all, and make another external <script> tag before the inline one to load it?

@DeMoorJasper
Copy link
Member Author

DeMoorJasper commented Jul 5, 2018

@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

@@ -62,6 +62,11 @@ const META = {
]
};

const SCRIPT_TYPES = {
'application/javascript': 'js',
'application/json': 'json'
Copy link
Member

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.

Copy link
Member Author

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

async postProcess(generated) {
// Hacky way of filtering out the css hmr JS
// Related: https://github.com/parcel-bundler/parcel/issues/1389
generated = generated.filter(
Copy link
Member

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.

async postProcess(generated) {
// Hacky way of filtering out the css hmr JS
// Related: https://github.com/parcel-bundler/parcel/issues/1389
generated = generated.filter(
Copy link
Member

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

parts.push({
type:
node.tag === 'style'
? 'css'
Copy link
Member

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

Copy link
Member Author

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
Copy link
Member

Choose a reason for hiding this comment

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

why was this removed?

Copy link
Member Author

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

@devongovett
Copy link
Member

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 require/import/@import don't work inside script and style tags might be weird for people. That would also be the hardest thing to implement...

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 require etc. at that time. In the mean time, we will still get the benefits of at least some of this.

@QingLeiLi
Copy link

@devongovett I`m glad to hear we have a plan to parcel2, anything else about the plan?

@DeMoorJasper
Copy link
Member Author

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() {
Copy link
Contributor

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

@DeMoorJasper
Copy link
Member Author

DeMoorJasper commented Jul 18, 2018

Should be working in most cases now. If anyone would like to test this, please do.

@tim-evans tim-evans mentioned this pull request Jul 18, 2018
@devongovett
Copy link
Member

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 @import in <style> tags, and import/require in <script> tags. For CSS, the imports are not inlined as usual, but modified to point to the correct URL for a separate bundle. For JS, we just throw an error saying imports and requires aren't supported in inline scripts yet. This should be clearer than just not working at runtime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants