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

issue-132 - dedupe insertStyle in output files #139

Merged
merged 3 commits into from
Jul 2, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
},
"scripts": {
"prepare": "npm run build && npm test",
"build": "npm run build-downlevel-dts && tsc --project tsconfig.json",
"build": "npm run build-downlevel-dts && tsc --project tsconfig.build.plugin.json && tsc --project tsconfig.build.insertStyle.json",
"build-downlevel-dts": "node scripts/clean-and-run-downlevel-dts.js",
"downlevel-dts": "downlevel-dts . ts3.5 [--to=3.5]",
"test": "nyc --reporter=html --reporter=text ava ./test/*.test.ts -s && npm run test:rollup.config.spec.ts",
Expand Down Expand Up @@ -54,7 +54,8 @@
],
"require": [
"ts-node/register"
]
],
"snapshotDir": "./test/snapshots"
},
"dependencies": {
"@rollup/pluginutils": "^3 || ^4 || ^5",
Expand Down
15 changes: 7 additions & 8 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import * as sass from 'sass';
import {dirname} from 'path';
import * as fs from 'fs';
import {createFilter} from '@rollup/pluginutils';
import {insertStyle} from './style';
import {
SassImporterResult,
RollupAssetInfo,
Expand Down Expand Up @@ -107,14 +106,20 @@ const MATCH_SASS_FILENAME_RE = /\.sass$/,
const out = JSON.stringify(resolvedCss);

let defaultExport = `""`;
let imports = '';

if (rollupOptions.insert) {
/**
* @see insertStyle.ts for additional information
* Let rollup handle import by processing insertStyle as a module
*/
imports = `import ${insertFnName} from '${__dirname}/insertStyle.js';\n`;
Copy link
Owner

@elycruz elycruz Jun 30, 2024

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).

Copy link
Collaborator Author

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 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.

image

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)

Copy link
Owner

@elycruz elycruz Jul 2, 2024

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 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).

Copy link
Collaborator Author

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):
image

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

Copy link
Owner

@elycruz elycruz Jul 2, 2024

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 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.

Copy link
Collaborator Author

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.

defaultExport = `${insertFnName}(${out});`;
} else if (!rollupOptions.output) {
defaultExport = out;
}

return `export default ${defaultExport};\n${restExports}`;
return `${imports}export default ${defaultExport};\n${restExports}`;
}); // @note do not `catch` here - let error propagate to rollup level
},

Expand Down Expand Up @@ -152,12 +157,6 @@ export = function plugin(options = {} as RollupPluginSassOptions): RollupPlugin
return {
name: 'rollup-plugin-sass',

intro() {
if (pluginOptions.insert) {
return insertStyle.toString().replace(/insertStyle/, insertFnName);
}
},

transform(code: string, filePath: string): Promise<any> {
if (!filter(filePath)) {
return Promise.resolve();
Expand Down
23 changes: 23 additions & 0 deletions src/insertStyle.ts
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;
}
21 changes: 0 additions & 21 deletions src/style.ts

This file was deleted.

3 changes: 3 additions & 0 deletions test/fixtures/multiple-entry-points/entryA.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import a from '../../assets/actual_a.scss';

export default a;
3 changes: 3 additions & 0 deletions test/fixtures/multiple-entry-points/entryB.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import b from '../../assets/actual_b.scss';

export default b;
64 changes: 58 additions & 6 deletions test/index.test.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
import test from 'ava';
import {promises as fs, constants as fsConstants} from 'fs';
import * as path from 'path';
import test from 'ava';
import sinon from 'sinon';
import {OutputOptions, rollup, RollupOutput} from 'rollup';
import * as sassJs from 'sass';
import sass from '../src/index';
import {SassOptions} from "../src/types";
import {error} from "../src/utils";
import postcss from "postcss";
import {extractICSS} from "icss-utils";

import sass from "../src/index";
import { SassOptions } from "../src/types";

const repoRoot = path.join(__dirname, '../'),

tmpDir = path.join(repoRoot, '.tests-output/'),
Expand Down Expand Up @@ -86,6 +86,22 @@ test('should import *.scss and *.sass files', async t => {
t.true([expectA, expectB, expectC].every(xs => rslt.includes(xs)));
});

test("should import *.scss and *.sass files with default configuration", async (t) => {
const outputBundle = await rollup({
input: "test/fixtures/basic/index.js",
plugins: [
sass(),
],
}),
{ output } = await outputBundle.generate({
format: "es",
file: path.join(tmpDir, "import_scss_and_sass_default_options.js"),
}),
rslt = squash(unwrap(output));

t.snapshot(rslt)
});

test('should compress the dest CSS', async t => {
const outputBundle = await rollup({
...baseConfig,
Expand Down Expand Up @@ -152,8 +168,44 @@ test('should insert CSS into head tag', async t => {
}),
{output} = await outputBundle.generate(generateOptions);

t.true(unwrap(output).includes('___$insertStyle("body{color:red}");'));
t.true(unwrap(output).includes('___$insertStyle("body{color:green}");'));
t.snapshot(unwrap(output));
});

test("should generate chunks with import insertStyle when `insert` is true", async (t) => {
const outputBundle = await rollup({
input: {
entryA: "test/fixtures/multiple-entry-points/entryA.js",
entryB: "test/fixtures/multiple-entry-points/entryB.js",
},
plugins: [
sass({
insert: true,
options: sassOptions,
}),
],
output: {
preserveModules: true,
preserveModulesRoot: "src",
},
external: [/\/insertStyle\.js$/],
});

const { output } = await outputBundle.generate(generateOptions);

t.is(output.length, 2, "has 2 chunks");
t.true(
output.every(
(chunk) =>
chunk.type === "chunk" &&
chunk.imports.some((it) => it.includes("/insertStyle.js"))
marcalexiei marked this conversation as resolved.
Show resolved Hide resolved
),
"each chunk must include insertStyle"
);

// outputBundle.write({
// format: 'es',
// dir: path.join(tmpDir, 'insert-style-preserve-modules'),
// });
});

test('should support output as function', async t => {
Expand Down
22 changes: 22 additions & 0 deletions test/snapshots/test/index.test.ts.md
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 added test/snapshots/test/index.test.ts.snap
Binary file not shown.
2 changes: 1 addition & 1 deletion test/style.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import test from 'ava';
import {readFileSync} from 'fs';
import {insertStyle} from '../src/style';
import insertStyle from '../src/insertStyle';
import jsdom from 'jsdom';

const expectA = readFileSync('test/assets/expect_a.css').toString();
Expand Down
8 changes: 8 additions & 0 deletions tsconfig.build.insertStyle.json
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"
}
}
6 changes: 6 additions & 0 deletions tsconfig.build.plugin.json
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"]
}
Loading