-
Notifications
You must be signed in to change notification settings - Fork 34
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
issue-132 - dedupe insertStyle in output files #139
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
/** | ||
* Create a style tag and append to head tag | ||
* | ||
* @warning this file is not included directly in the source code! | ||
* If user specifies inject option to true, an import to this file will be injected in rollup output. | ||
* Due to this reason this file is compiled into a ESM module separated from other plugin source files. | ||
* That is the reason of why there are two tsconfig.build files. | ||
* | ||
* @return css style | ||
*/ | ||
export default function insertStyle(css: string | undefined): string | undefined { | ||
if (!css || typeof window === 'undefined') { | ||
return; | ||
} | ||
|
||
const style = document.createElement('style'); | ||
style.setAttribute('type', 'text/css'); | ||
style.innerHTML = css; | ||
|
||
document.head.appendChild(style); | ||
|
||
return css; | ||
} |
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
import a from '../../assets/actual_a.scss'; | ||
|
||
export default a; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
import b from '../../assets/actual_b.scss'; | ||
|
||
export default b; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
# Snapshot report for `test/index.test.ts` | ||
|
||
The actual snapshot is saved in `index.test.ts.snap`. | ||
|
||
Generated by [AVA](https://avajs.dev). | ||
|
||
## should import *.scss and *.sass files with default configuration | ||
|
||
> Snapshot 1 | ||
|
||
'var atualA = "body {\\n color: red;\\n}";var atualB = "body {\\n color: green;\\n}";var atualC = "body {\\n color: blue;\\n}";var index = atualA + atualB + atualC;export { index as default };' | ||
|
||
## should insert CSS into head tag | ||
|
||
> Snapshot 1 | ||
|
||
`import ___$insertStyle from '../../../src/insertStyle.js';␊ | ||
␊ | ||
___$insertStyle("body{color:red}");␊ | ||
␊ | ||
___$insertStyle("body{color:green}");␊ | ||
` |
Binary file not shown.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
/* @see insertStyle.ts for additional information */ | ||
{ | ||
"extends": "./tsconfig.json", | ||
"include": ["./src/insertStyle.ts"], | ||
"compilerOptions": { | ||
"module": "ES6" | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
/* @see insertStyle.ts for additional information */ | ||
{ | ||
"extends": "./tsconfig.json", | ||
"include": ["./src/*"], | ||
"exclude": ["./src/insertStyle.ts"] | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Path here should be
rollup-plugin-sass/dist/insertStyle
instead (since files generated here are consumed on the user-land side).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.
Unfortunately this is not feasible:
if we use
rollup-plugin-sass/dist/insertStyle
as pathrollup
won't be able to resolve it.This because
rollup
doesn't resolve imports with node by default, to do so you neednode-resolve
plugin.Since the import is not resolved,
rollup
will not include the insertStyle code in the bundle.On the contrary using
__dirname
will fill the import with an absolute path thatrollup
can resolve correctly.__dirname
points todist
folder which also containinsertStyle
so there should be no problem using it.(I already tested this in my app and is working)
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.
~~Right though, to your first point, shouldn't we assume that anyone that wants modules from
node_modules/
to be included in their app bundles, would add additional configuration for said modules to be included? (something like@rollup/plugin-node-resolve
) ~~ - Read this too early in the morning - I get what you're saying aboutnode-resolve
- Is there a way we can get the relative path to the file instead? - Revealing the users system path in artifacts is a code smell and reveals information that could be used against the authors.To your second point, you are correct, however, doing this causes the users system path to be revealed in their artifacts when addingexternal: [/\/insertStyle\.js$/]
to a rollup config - see test file (in PR).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.
Interestingly the complete file system path is not inlined when using the plugin via
node_modules
.I did a test on my work app (using
rollup@4.12.1
):Anyway I'll try to use relative path. Maybe something could be achieved via
path.relative
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.
~~Ok, gotcha, no need though, seems the full path only gets rendered when
external: [/\/insertStyle\.js$/]
is set (in the app's rollup config, which wouldn't make sense ifinsertStyle
is required for an app, in this case). ~~I would say let's setup an 'examples/using-preserve-modules' app example which we could use to validate the behavior, and allow others to do the same, this way we can ensure that everything will work as expected (opening a ticket and PR for this shortly).Disregard comments above - Just ran an additional test and indeed when when
insertStyle
isn't excluded viaexternal: [/\/insertStyle\.js$/]
module path doesn't include the (OS) absolute path - Moving ahead with merge.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.
Sorry, I must have left the
external
setting during some tryouts I've done earlier 😅.I'll see if I can remove it when migrating to snapshots.