- Notifications
You must be signed in to change notification settings - Fork72
Support thread loader by optional inline emitCss (base64, querystring)#164
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| @@ -272,6 +272,29 @@ module.exports = { | |||
| } | |||
| ``` | |||
| ## Using svelte-loader in combination with thread-loader | |||
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.
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 mentioningthread-loader, but it maybe shouldn't be the focus
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.
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 ifthread-loader is used in a pipeline withsvelte-loader and turn this on automatically.
Maybe we should make it that way.. 🤔
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.
That might be nicer than adding an option. But I also wonder if it's even worth adding something that's only useful forthread-loader
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.
Part of the reason I'm here is because one day one man tried to usesvelte-loader withtailwindcss throughsvelte-preprocess and had 4 mins of build-time and 40 seconds of reload time for 20 or so components. 😁
That would save him.
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.
Does that mean that you need to usethread-loader to deal withsveltejs/svelte-preprocess#275 ?
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.
Exactly. It will speed up that thing significantly.
But you also need to configure preprocess and webpack.css pipeline correctly to gain maximum performance.
trash-and-fireJan 17, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
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.
Now it checks automatically. |
| @@ -62,10 +65,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; | |||
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.
Perhaps there should be an override to force inline mode to always be enabled or disabled? (this could be written asoptions.forceInlineCss ?? this.emitFile === undefined in Node 14+ but alas versions below thatdon't support the?? operator)
| constthreadLoaderUsed=this.emitFile===undefined; | |
| constthreadLoaderUsed=(options.forceInlineCss===undefined) ?this.emitFile===undefined :options.forceInlineCss; |
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.
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
So what do you think guys? |
6680419 toe353a4cCompareInteresting, it doesn't work with So I guess we will release without this PR. |
This option makes svelte-loader output base64'd css in querystring,which is always available to any thread.It is turned off by default because it pollutes webpack's consoleoutput, but if user needs to use thread-loader, he can.
e353a4c tob2ca22cCompare
This option makes svelte-loader output base64'd css in querystring, which is always available to any thread.
It is turned off by default because it pollutes webpack's console output, but if user needs to use thread-loader, he can.