-
Notifications
You must be signed in to change notification settings - Fork 71
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
Support thread loader by optional inline emitCss (base64, querystring) #164
base: master
Are you sure you want to change the base?
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 mentioning thread-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 if thread-loader
is used in a pipeline with svelte-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 for 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.
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.
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 use thread-loader
to deal with sveltejs/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.
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 as options.forceInlineCss ?? this.emitFile === undefined
in Node 14+ but alas versions below that don't support the ??
operator)
const threadLoaderUsed = this.emitFile === undefined; | |
const threadLoaderUsed = (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
to
e353a4c
Compare
Interesting, 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 console output, but if user needs to use thread-loader, he can.
e353a4c
to
b2ca22c
Compare
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.