issue-132 - dedupe insertStyle in output files

[marcalexiei]

Thanks for the work on this plugin!

Description

This PR aims to remove the duplication of insertStyle function in each chunk / bundle produced by rollup

• fixes fix: Ensure __$insertStyle function is inserted only once per bundle #132
• fixes insert option + chunks places ___$insertStyle function in every chunked file #69

Proposed solution

When insert option is set to true,
instead of adding the code of insertStyle on the top of each file via the intro hook,
the insertStyle function is now included via an import which points to src/insertStyle.ts file.

Note

Using the import, rollup will take care to include the function code only once per bundle (or output only one file if preserveModules function is used).

Important

Since rollup process ESModules the insertStyle file should not be compiled using CommonJS modules but using ES6 modules.
Because of this I added 2 tsconfig.build files to produce the two different outputs.

Tip

When migrating this package to ESM these two files can be safely removed.

[elycruz]

Well done sir! (👏) Good work !!

Writing some unit tests - unless you already are?
Will write some tests for it.

[marcalexiei]

In order to test this scenario I updated 'should insert CSS into head tag' test to better match that insertStyle is included once.

In addition I also added a new test: 'should import *.scss and *.sass files with default configuration' to get rid of coverage decrease error.

Note

Since we are basically testing strings, for the two tests involved in this PR, I opted out to use snapshot testing.
The snapshots are stored inside test/snapshots folder, let me know if I need to change their location.

Tip

I think we can migrate all tests to snapshots system,
however I prefer to do it in a separate PR to not clutter this one too much.

[elycruz]

Updates look good minus a couple of call outs I listed:

[elycruz]

Path here should be rollup-plugin-sass/dist/insertStyle instead (since files generated here are consumed on the user-land side).

[marcalexiei]

Unfortunately this is not feasible:

if we use rollup-plugin-sass/dist/insertStyle as path rollup won't be able to resolve it.
This because rollup doesn't resolve imports with node by default, to do so you need node-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 that rollup can resolve correctly.
__dirname points to dist folder which also contain insertStyle so there should be no problem using it.
(I already tested this in my app and is working)

[elycruz]

~~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 about node-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 adding external: [/\/insertStyle\.js$/] to a rollup config - see test file (in PR).

[marcalexiei]

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 [email protected]):

Anyway I'll try to use relative path. Maybe something could be achieved via path.relative

[elycruz]

~~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 if insertStyle 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 via external: [/\/insertStyle\.js$/] module path doesn't include the (OS) absolute path - Moving ahead with merge.

[marcalexiei]

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.

[elycruz]

Thank you for the good work @marcalexiei 👍 .