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

remove node-sass dependency #10797

Merged
merged 1 commit into from
Apr 2, 2021
Merged

remove node-sass dependency #10797

merged 1 commit into from
Apr 2, 2021

Conversation

brad-decker
Copy link
Contributor

Back in #10208 I changed us over to using dart-sass instead of node-sass, but gulp-sass had a dependency on node-sass that still required it in our build. This change uses gulp-dart-sass to remove that element.

@brad-decker
Copy link
Contributor Author

Do i need to just delete the patch file to get prep-deps passing? also there are lavamoat policies for node-sass that should be removed but wanted to get a thumbs up / greenlight before doing that @kumavis / @EtDu

@Gudahtt
Copy link
Member

Gudahtt commented Apr 1, 2021

@brad-decker Instructions on how to manage lavamoat policies and allow-scripts configuration are here: https://github.com/MetaMask/metamask-extension#changing-dependencies

And yes, deleting the patch would be the correct approach. That reminds me - we don't document how to deal with patches yet 🤔 I should address that.

@brad-decker
Copy link
Contributor Author

brad-decker commented Apr 1, 2021

Erg... the policy file changes made me look at bundles

https://bundlephobia.com/result?p=gulp-dart-sass@1.0.2 vs https://bundlephobia.com/result?p=gulp-sass@4.1.0

2.2mb minified vs 141kb minified... I'm thinking we just keep node-sass around for a while.

edit: Well, it's mostly sass which we already include. shrug

@brad-decker brad-decker closed this Apr 1, 2021
@brad-decker brad-decker reopened this Apr 1, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Apr 1, 2021
@Gudahtt
Copy link
Member

Gudahtt commented Apr 1, 2021

That is misleading because of node-sass, which downloads a binary as part of the install script I think? Thus hiding the true size.

Plus the bulk of the size of the dart package is due to the sass package, which we already have in our dependency tree. I do think this would be a strict reduction in our dependencies.

@metamaskbot
Copy link
Collaborator

Builds ready [41e4529]
Page Load Metrics (591 ± 28 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint448359105
domContentLoaded3616415895828
load3626425915728
domInteractive3606415895828

@brad-decker brad-decker marked this pull request as ready for review April 1, 2021 16:41
@brad-decker brad-decker requested review from kumavis and a team as code owners April 1, 2021 16:41
@MetaMask MetaMask unlocked this conversation Apr 1, 2021
kumavis
kumavis previously approved these changes Apr 2, 2021
Copy link
Member

@kumavis kumavis left a comment

Choose a reason for hiding this comment

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

looks good. huge yarn-lock reduction 👀 👍

@Gudahtt
Copy link
Member

Gudahtt commented Apr 2, 2021

I have just rebased this to fix the yarn.lock conflict. It resolved itself automatically after rebasing and running yarn setup.

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@metamaskbot
Copy link
Collaborator

Builds ready [da4952d]
Page Load Metrics (642 ± 66 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint459359126
domContentLoaded407108764013766
load408108964213766
domInteractive407108764013766

@Gudahtt Gudahtt merged commit f5c8984 into develop Apr 2, 2021
@Gudahtt Gudahtt deleted the remove-node-sass-dep branch April 2, 2021 14:27
@github-actions github-actions bot locked and limited conversation to collaborators Apr 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants