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

Source maps and minification for dynamic CSS modules. #9998

Merged
merged 1 commit into from Jun 15, 2018

Conversation

@benjamn
Copy link
Member

@benjamn benjamn commented Jun 15, 2018

When a CSS (or compiled-to-CSS) module is lazy (e.g., in imports/ or node_modules) and not otherwise imported by another CSS module, Meteor automatically turns it into a JS module so that it can be handled by the ImportScanner, and imported dynamically by other JS modules.

Until now, there were two problems with CSS handled in this way: it did not have proper source maps, and the CSS text was not minified in production.

This commit introduces a special minification step for dynamic CSS, which must take place before we create the dynamic JS module. Waiting for the usual minification of CSS would be a mistake, since that happens long after the ImportScanner and Linker have already processed JavaScript modules. Modifying the contents of JS modules at that point would be impossible without recomputing source maps, etc.

Since the JS module dynamically creates a <style> tag and appends it to the <head> of the document, the code of the JS module has no meaningful relationship to the lines of CSS text that are actually evaluated by the browser, so it would be a mistake to give the JS module the same source map as the original CSS resource.

Instead, when there is a source map, we write it out as an asset that can be requested at runtime, and append a sourceMappingURL comment to the end of the CSS text referring to this asset URL. Note that this only happens in development, which makes sense because minification in production invalidates the source map, and we don't want to expose source code in production anyway.

@sebakerckhof
Copy link
Contributor

@sebakerckhof sebakerckhof commented Jun 15, 2018

This is pretty awesome.

When a CSS (or compiled-to-CSS) module is lazy (e.g., in `imports/` or
`node_modules`) and not otherwise imported by another CSS module, Meteor
automatically turns it into a JS module so that it can be handled by the
`ImportScanner`, and imported dynamically by other JS modules.

Until now, there were two problems with CSS handled in this way: it did
not have proper source maps, and the CSS text was not minified in
production.

This commit introduces a special minification step for dynamic CSS, which
must take place before we create the dynamic JS module. Waiting for the
usual minification of CSS would be a mistake, since that happens long
after the `ImportScanner` and `Linker` have already processed JavaScript
modules. Modifying the contents of JS modules at that point would be
impossible without recomputing source maps, etc.

Since the JS module dynamically creates a `<style>` tag and appends it to
the `<head>` of the document, the code of the JS module has no meaningful
relationship to the lines of CSS text that are actually evaluated by the
browser, so it would be a mistake to give the JS module the same source
map as the original CSS resource.

Instead, when there is a source map, we write it out as an asset that can
be requested at runtime, and append a sourceMappingURL comment to the end
of the CSS text referring to this asset URL. Note that this only happens
in development, which makes sense because minification in production
invalidates the source map, and we don't want to expose source code in
production anyway.
@benjamn benjamn force-pushed the source-maps-and-minification-for-dynamic-css branch from 35077ad to 6fb9be1 Jun 15, 2018
@benjamn benjamn merged commit ce34a60 into devel Jun 15, 2018
19 checks passed
19 checks passed
CLA Author has signed the Meteor CLA.
Details
ci/circleci: Clean Up Your tests passed on CircleCI!
Details
ci/circleci: Docs Your tests passed on CircleCI!
Details
ci/circleci: Get Ready Your tests passed on CircleCI!
Details
ci/circleci: Isolated Tests Your tests passed on CircleCI!
Details
ci/circleci: Test Group 0 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 1 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 10 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 2 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 3 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 4 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 5 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 6 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 7 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 8 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 9 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@pagesrichie
Copy link

@pagesrichie pagesrichie commented Jun 15, 2018

@benjamn Does release 1.7.1-beta.3 here contain the fix for this (the whole fast less compiling issue and the crashing error)?
https://github.com/meteor/meteor/releases/tag/release%2FMETEOR%401.7.1-beta.3

@pagesrichie
Copy link

@pagesrichie pagesrichie commented Jun 15, 2018

@benjamn just tried the 1.7.1-beta.3 . While it works with just one less file "main.less" added in client dir, if I add the standard semantic-ui folder under client dir with all the less files.. it crashes with the following error:

/Users/macpc/.meteor/packages/static-html/.1.2.2.wp43cd.99etr++os+web.browser+web.cordova/plugin.compileStaticHtmlBatch.os/npm/node_modules/meteor/promise/node_modules/meteor-promise/promise_server.js:190
throw error;
^

TypeError: First argument must be a string, Buffer, ArrayBuffer, Array, or array-like object.
at Function.Buffer.from (buffer.js:183:11)
at CssOutputResource.get (/tools/isobuild/compiler-plugin.js:932:23)
at CssOutputResource.get data [as data] (/tools/isobuild/compiler-plugin.js:910:28)
at resources.forEach.resource (/tools/isobuild/bundler.js:1209:28)
at Array.forEach ()
at sourceBatches.forEach.sourceBatch (/tools/isobuild/bundler.js:1182:17)
at Array.forEach ()
at ClientTarget.emitResources (/tools/isobuild/bundler.js:1117:19)
at buildmessage.enterJob (/tools/isobuild/bundler.js:840:12)
at /tools/utils/buildmessage.js:359:18
at exports.EnvironmentVariable.withValue (/tools/utils/fiber-helpers.js:89:14)
at /tools/utils/buildmessage.js:352:34
at exports.EnvironmentVariable.withValue (/tools/utils/fiber-helpers.js:89:14)
at /tools/utils/buildmessage.js:350:23
at exports.EnvironmentVariable.withValue (/tools/utils/fiber-helpers.js:89:14)
at Object.enterJob (/tools/utils/buildmessage.js:324:26)
at ClientTarget.make (/tools/isobuild/bundler.js:828:18)
at /tools/isobuild/bundler.js:3066:14
at /tools/isobuild/bundler.js:3155:20
at Array.forEach ()
at Function.
.each.
.forEach (/Users/macpc/.meteor/packages/meteor-tool/.1.7.1-beta.3.1lkjoui.xrva++os.osx.x86_64+web.browser+web.browser.legacy+web.cordova/mt-os.osx.x86_64/dev_bundle/lib/node_modules/underscore/underscore.js:79:11)
at /tools/isobuild/bundler.js:3154:7
at /tools/utils/buildmessage.js:271:13
at exports.EnvironmentVariable.withValue (/tools/utils/fiber-helpers.js:89:14)
at /tools/utils/buildmessage.js:264:29
at exports.EnvironmentVariable.withValue (/tools/utils/fiber-helpers.js:89:14)
at /tools/utils/buildmessage.js:262:18
at exports.EnvironmentVariable.withValue (/tools/utils/fiber-helpers.js:89:14)
at /tools/utils/buildmessage.js:253:23
at exports.EnvironmentVariable.withValue (/tools/utils/fiber-helpers.js:89:14)
at Object.capture (/tools/utils/buildmessage.js:252:19)
at bundle (/tools/isobuild/bundler.js:3047:31)
at files.withCache (/tools/isobuild/bundler.js:2994:32)
at Object.withCache (/tools/fs/files.js:1710:12)
at Object.exports.bundle (/tools/isobuild/bundler.js:2994:16)
at Profile.run (/tools/runners/run-app.js:579:36)
at Function.run (/tools/tool-env/profile.js:490:12)
at bundleApp (/tools/runners/run-app.js:578:34)
at AppRunner._runOnce (/tools/runners/run-app.js:622:35)
at AppRunner._fiber (/tools/runners/run-app.js:880:28)
at /tools/runners/run-app.js:408:12
npm ERR! code ELIFECYCLE
npm ERR! errno 1

I revert back to meteor 1.7.0.1 and the error disappears, css works, back to slow LESS compiling however.

@fabien-h
Copy link

@fabien-h fabien-h commented Jul 15, 2018

Hi, great news. Works perfectly in dev and prod.

What about auto-prefixing? Are we going to get that through a plugin or will it be built in?

@yorrd
Copy link

@yorrd yorrd commented Jul 17, 2018

this is looking very interesting. Some input concerning web components:

Due to the encapsulated nature of CSS in a web component, injecting anything into the <head> will likely mean that anyone using web components will complain about this sooner or later. Idea on how to fix this, maybe this has already been in discussion and I missed it:

Just generate a js file which includes some arbitrary string at the beginning and another one at the end which you can then import from your component. For example, my current solution for importing external css is this:

import { html } from '../base';

export default html`
<style>
  * {
    color: red;
  }
</style>
`;

So I would need to prepend anything until the closing > of <style> and append </style>';.

Right now I have a superclass which handles this, but I can't use real css files.

Bonus question: how to make those work with barbatus:typescript or a native typescript compiler?

EDIT: I realize this is not zero-config. Probably a job for a plugin...

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

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.