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

fix(@angular-devkit/build-angular): Sass compilation in StackBlitz webcontainers #22294

Merged
merged 1 commit into from
Dec 6, 2021
Merged

fix(@angular-devkit/build-angular): Sass compilation in StackBlitz webcontainers #22294

merged 1 commit into from
Dec 6, 2021

Conversation

alan-agius4
Copy link
Collaborator

When process.versions.webcontainer is a string it means that we are running in a StackBlitz webcontainer. SassWorkerImplementation uses receiveMessageOnPort Node.js worker_thread API to ensure sync behavior which is ~2x faster. However, it is non trivial to support this in a webcontainer and while slower we choose to use dart-sass which in Webpack uses the slower async path.

// cc @markwhitfeld

@alan-agius4 alan-agius4 added the target: patch This PR is targeted for the next patch release label Dec 3, 2021
@google-cla google-cla bot added the cla: yes label Dec 3, 2021
Copy link

@markwhitfeld markwhitfeld left a comment

Choose a reason for hiding this comment

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

That check looks good 👍

@d3lm
Copy link
Contributor

d3lm commented Dec 3, 2021

Thanks so much for this quick implementation and for adding an env check here for the time being. I am actually gonna look into this very soon so we can support receiveMessageOnPort at least minimally. Thanks a lot!

Copy link
Collaborator

@dgp1130 dgp1130 left a comment

Choose a reason for hiding this comment

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

Just for context, do we only use dart-sass for webcontainers, or are there other usages were it is still supported? I'm just wondering if this is the major thing blocking us from moving to node-sass, or if there are other problems that need to be resolved anyways?

@alan-agius4
Copy link
Collaborator Author

Just for context, do we only use dart-sass for webcontainers, or are there other usages were it is still supported? I'm just wondering if this is the major thing blocking us from moving to node-sass, or if there are other problems that need to be resolved anyways.

We only support dart-sass.

@dgp1130
Copy link
Collaborator

dgp1130 commented Dec 3, 2021

We only support dart-sass.

Ah thanks, I was getting the direction of node-sass -> dart-sass backwards, and thought this might be related. Disabling the worker in webcontainers is reasonable until it has the necessary support.

…bcontainers

When `process.versions.webcontainer` is truthy it means that we are running in a StackBlitz webcontainer. `SassWorkerImplementation` uses `receiveMessageOnPort` Node.js `worker_thread` API to ensure sync behavior which is ~2x faster. However, it is non trivial to support this in a webcontainer and while slower we choose to use `dart-sass` which in Webpack uses the slower async path.
@alan-agius4 alan-agius4 added the action: merge The PR is ready for merge by the caretaker label Dec 4, 2021
@filipesilva filipesilva merged commit ac66e40 into angular:master Dec 6, 2021
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jan 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants