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.

Sign up for GitHub

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

Support thread loader by optional inline emitCss (base64, querystring) #164

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Support thread loader by optional inline emitCss (base64, querystring) #164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
14c0824
Support thread loader by optional inline emitCss (base64, querystring)
non25 Jan 17, 2021
94773b4
Determine if thread-loader is enabled by checking if emitFile is avai…
non25 Jan 17, 2021
e8ee41d
Fix test
non25 Jan 17, 2021
b2ca22c
Adjust readme
non25 Jan 17, 2021
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
10 changes: 10 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,16 @@ If you are using autoprefixer for `.css`, then it is better to exclude emitted c

This ensures that global css is being processed with `postcss` through webpack rules, and svelte component's css is being processed with `postcss` through `svelte-preprocess`.

## Using svelte-loader in combination with thread-loader
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this feature be useful in other cases? (e.g. I think you mentioned tailwind and babel). If so, can we describe this feature more generically? I'm find mentioning thread-loader, but it maybe shouldn't be the focus

Copy link
Contributor Author

@non25 non25 Jan 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't think of other cases... It is mainly useful because it allows other processess/workers to get css reliably, and not try to access an empty Map.
The default Map storage with file paths in a query will only be accessible in that exact process.
@trash-and-fire told me that we can detect if thread-loader is used in a pipeline with svelte-loader and turn this on automatically.
Maybe we should make it that way.. 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That might be nicer than adding an option. But I also wonder if it's even worth adding something that's only useful for thread-loader

Copy link
Contributor Author

@non25 non25 Jan 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of the reason I'm here is because one day one man tried to use svelte-loader with tailwindcss through svelte-preprocess and had 4 mins of build-time and 40 seconds of reload time for 20 or so components. 😁
That would save him.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that mean that you need to use thread-loader to deal with sveltejs/svelte-preprocess#275 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly. It will speed up that thing significantly.
But you also need to configure preprocess and webpack .css pipeline correctly to gain maximum performance.

Copy link

@trash-and-fire trash-and-fire Jan 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not an entirely accurate statement.

There was a problem of rebuilding all virtual css files if only one changed. This issue has been fixed without using a thread loader. A thread loader can provide additional performance by parallelizing the compilation of each file into a process pool. But there is no shared memory between processes, we must transfer all the content of the css file through the import line.


By default `svelte-loader` uses a Map to store css, and passes keys to that Map through custom loader string in query parameter.

This won't work for multiple `thread-loader` processess. `css-loader` won't find component's css in a Map that is located in other process.

If you set up `thread-loader` on top of `svelte-loader` however, it will pass whole base64'd css in a query, without using Map.

It will clutter the console output, but you will gain compilation speed, especially when using `tailwindcss` with `@apply` through `svelte-preprocess`.

## License

MIT
22 changes: 16 additions & 6 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,12 @@ module.exports = function(source, map) {
const options = { ...getOptions(this) };
const callback = this.async();

if (options.cssPath) {
const css = virtualModules.get(options.cssPath);
virtualModules.delete(options.cssPath);
if (options.cssPath || options.cssData) {
const css = options.cssData
? Buffer.from(options.cssData, 'base64').toString('utf-8')
: virtualModules.get(options.cssPath);
if (options.cssPath)
virtualModules.delete(options.cssPath);
callback(null, css);
return;
}
Expand Down Expand Up @@ -66,10 +69,17 @@ module.exports = function(source, map) {

if (options.emitCss && css.code) {
const resource = posixify(compileOptions.filename);
const cssPath = `${resource}.${index++}.css`;
const threadLoaderUsed = this.emitFile === undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps there should be an override to force inline mode to always be enabled or disabled? (this could be written as options.forceInlineCss ?? this.emitFile === undefined in Node 14+ but alas versions below that don't support the ?? operator)

Suggested change
const threadLoaderUsed = this.emitFile === undefined;
const threadLoaderUsed = (options.forceInlineCss === undefined) ? this.emitFile === undefined : options.forceInlineCss;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a reasonable suggestion, but the downside is that the option then becomes part of the API, which makes it hard to change (we'd need another major release). I might wait until a need for it arises to leave us more flexibility instead of adding it up front

const cssPath = threadLoaderUsed
? `${resource}.css`
: `${resource}.${index++}.css`;
const cssQuery = threadLoaderUsed
? `cssData=${Buffer.from(css.code).toString('base64')}`
: `cssPath=${cssPath}`;
css.code += '\n/*# sourceMappingURL=' + css.map.toUrl() + '*/';
js.code += `\nimport '${cssPath}!=!svelte-loader?cssPath=${cssPath}!${resource}'\n;`;
virtualModules.set(cssPath, css.code);
js.code += `\nimport '${cssPath}!=!svelte-loader?${cssQuery}!${resource}'\n;`;
if (!threadLoaderUsed)
virtualModules.set(cssPath, css.code);
}

callback(null, js.code, js.map);
Expand Down
2 changes: 1 addition & 1 deletion test/loader.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ describe('loader', () => {
function(err, code, map) {
expect(err).not.to.exist;

expect(code).to.match(/!=!svelte-loader\?cssPath=/);
expect(code).to.match(/!=!svelte-loader\?cssData=/);
},
{ emitCss: true }
)
Expand Down