-
-
Notifications
You must be signed in to change notification settings - Fork 609
feat: add externals support for CSS imports #496
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #496 +/- ##
=========================================
+ Coverage 98.36% 98.46% +0.1%
=========================================
Files 10 11 +1
Lines 366 392 +26
Branches 87 94 +7
=========================================
+ Hits 360 386 +26
Misses 6 6
Continue to review full report at Codecov.
|
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.
Good direction. Docs + tests and it's good to go.
@bebraw: awesome! I will add docs + tests very soon and update here again :D |
@fahad19 Does this only work with when |
@michael-ciniawsky: I cannot confirm it yet without tests. but manually, I only tested it with the |
Where do the externals come from? Could you give an example |
That's important, since we plan to clearly separate CSS Modules and CSS loading in the future due to perf problems in the current loader, if it works with modules exclusively we need to refactor that to |
@michael-ciniawsky If this is indeed CSS Modules specific, probably the smartest move would be to wait till the split happens and then push this functionality to the CSS Modules specific loader. |
Externals are global objects. For e.g., if <script src="css-framework.js"></script> It would expose itself as // css-framework/index.js - entry for bundle
import variables from './variables.css';
import typography from './typography.css';
export {
variables,
typography,
}; And the CSS files would be: // css-frameworok/variables.css
@value primary-color: DodgerBlue;
@value secondary-color: DeepSkyBlue;
// css-framework/typography.css
.bold {
font-weight: bold;
}
.italic {
font-style: italic;
} The object in the browser would look like this: // console.log(window.CssFramework);
{
"variables": {
"primary-color": "DodgerBlue",
"secondary-color": "DeepSkyBlue"
},
"typography": {
"bold": "typography__bold___2AHac",
"italic": "typography__italic___3uoRc"
}
} And the final output of When other bundles do For other bundles, the webpack config for // some-other-app/webpack.config.js
{
modules: true,
localIdentName: '[name]__[local]___[hash:base64:5]',
externals: {
'css-framework/variables.css': 'CssFramework.variables',
'css-framework/typography.css': 'CssFramework.typography',
}
} And an example css file may look like: // some-other-app/styles.css
@value variables: "css-framework/variables.css";
@value primary-color, secondary-color from variables;
.someClass {
composes: bold from "css-framework/typography.css";
color: primary-color;
} |
currently, my needs are |
* master: fix: case insensitivity of @import (webpack-contrib#514) chore: update comment (webpack-contrib#515) docs(README): improve importLoaders section and example (webpack-contrib#512) docs(README): fix link (webpack-contrib#508) docs(README): fix typo in example (webpack-contrib#507) docs(README): fix typo in maintainers link (webpack-contrib#505) fix: imported variables are replaced in exports if followed by a comma (webpack-contrib#504) docs(README): standardize (webpack-contrib#503) test: `charset` directive (webpack-contrib#502) fix: url with a trailing space is now handled correctly (webpack-contrib#494) fix: use `btoa` instead `Buffer` (webpack-contrib#501) test: add test for escaped characters (webpack-contrib#493) test: add tests for encoded svg data uri (webpack-contrib#492) test: add tests when css contain data uri and source maps are enabled (webpack-contrib#491) fix: loader now correctly handles `url` with space(s) (webpack-contrib#495)
The feature being added does not require |
conflicts taken care of. up for final review. |
lib/loader.js
Outdated
var sourceMap = query.sourceMap || false; | ||
var resolve = createResolver(query.alias); | ||
|
||
var givenExternals = query.externals || (this.externals || {}); |
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 logic would work better in a function of its own.
lib/loader.js
Outdated
|
||
var externalsKeys = Object.keys(externals); | ||
|
||
function findExternalFromImportUrl(importUrl) { |
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 would push this function outside to root scope.
@fahad19 Also hold on with this a bit longer, I have the groundwork for the next major of |
@bebraw: feedback taken care of. @michael-ciniawsky: sure. please let me know when you are ready. |
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 stand corrected about the modules. Regardless of that, all pull requests > semver: Patch
are being held until Tobias is finished with https://github.com/webpack-contrib/css-loader/tree/new-loader for the sake of saving his sanity.
Given the nature of the change, once merged this feature will exist initially in the existing Major
release and we can discuss if it makes sense in the 1.0.0
version once it's stabilized.
Yep same as in #421 for the meantime |
@d3viant0ne: thanks for the explanation! just to clarify again, this new |
Sry but I have to declined this PR for now, since |
What kind of change does this PR introduce?
New feature via
externals
option incss-loader
, much like Webpack's core externals feature.Did you add tests for your changes?
Yes.
If relevant, did you update the README?
Yes.
Summary
I opened an issue in
css-modules
repository for this first: css-modules/css-modules#227To summarise, I want a setup like this with FrintJS:
Basically, I don't want Frint App bundles to include any CSS Framework code, since it has already been loaded before.
And my Webpack config for bundling Frint Apps would look like this for
css-loader
:In any of the App bundles, when I do this:
And my
styles.css
imports something fromcss-framework
package:Instead of including the
css-framework
code in the bundle, it would get the values dynamically in the browser from the globalwindow.CssFramework.variables
.Does this PR introduce a breaking change?
Possibly for old node versions, but I am happy to update them.
Other information
Fully working proof-of-concept with FrintJS here: https://github.com/fahad19/frint-css-poc