feat(css): add exportType style to inject styles into DOM#20579
feat(css): add exportType style to inject styles into DOM#20579alexander-akait merged 7 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: c9535d2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
4108e90 to
91c6e2c
Compare
|
This PR is packaged and the instant preview is available (915293e). Install it locally:
npm i -D webpack@https://pkg.pr.new/webpack@915293e
yarn add -D webpack@https://pkg.pr.new/webpack@915293e
pnpm add -D webpack@https://pkg.pr.new/webpack@915293e |
| ]), | ||
| "}" | ||
| ]), | ||
| "}", |
There was a problem hiding this comment.
I think we don't need here options.attributes, let's add a hook like we have LoadScriptRuntimeModule - createScript, let's add here createStyle and pass the code to hook,
Also let's set:
'script.setAttribute("data-webpack", dataWebpackPrefix + key);'
just look at LoadScriptRuntimeModule, key is module id in our case
| "};" | ||
| ]), | ||
| "}", | ||
| "", |
There was a problem hiding this comment.
I think we need to add a check here, is target universal we should generate it, if web we should not generate it, because in web we always have document
| "var item = list[i];", | ||
| "var identifier = item[0];", | ||
| "", | ||
| "var indexByIdentifier = findIndex(identifier);", |
There was a problem hiding this comment.
We can rewrite it and improve perfomance using data-webpack-module-id data attribute, so we will no need to go all style tags, we already will know what we should remove or update, the same for other part of generated code
| ]), | ||
| "}", | ||
| "", | ||
| "function domAPI(options) {", |
There was a problem hiding this comment.
Maybe we can simplify domAPI and avoid this function at all injection, logic there we need to call it, we have it in style-loader due we don't have hooks and we will need to pass options from style-loader, here we have hooks so maybe things can be simplified and if somebody want to modify logic we will just introduce more hooks
alexander-akait
left a comment
There was a problem hiding this comment.
Looks very good, let's improve couple things and rebase and we can merge
d61cba0 to
786dbd0
Compare
|
@alexander-akait Thanks for the great suggestion |
| ), | ||
| "", | ||
| "var head = document.head || document.getElementsByTagName('head')[0];", | ||
| "head.appendChild(element);", |
There was a problem hiding this comment.
let's use document.head.appendChild(style), also let's rename to style, we are using this pattern everywhere, not sure why we have this fallback in style-loader
alexander-akait
left a comment
There was a problem hiding this comment.
And let's add target: ["node", "web"] here, we should export CSS modules classes and no actions with DOM, like we are doing with link
|
And we can merge, thank you ❤️ |
Types CoverageCoverage after merging feat/css_style into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Summary
feat(css): add exportType style to inject styles into DOM
What kind of change does this PR introduce?
feat
Did you add tests for your changes?
Yes
Does this PR introduce a breaking change?
No
If relevant, what needs to be documented once your changes are merged or what have you already documented?
update exportType options