[Manual Backport 2.x] Replace node-sass with sass-embedded (#5338)
#5593
Conversation
AMoo-Miki
requested review from
ananzh,
kavilla,
ashwin-pc,
joshuarrrr,
abbyhu2000,
zengyan-amazon,
kristenTian,
zhongnansu,
manasvinibs,
ZilongX,
Flyingliuhub and
BSFishy
as code owners
AMoo-Miki
changed the title
node-sass with sass-embedded (#5338)node-sass with sass-embedded (#5338)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 2.x #5593 +/- ##
==========================================
- Coverage 66.98% 66.94% -0.04%
==========================================
Files 3287 3287
Lines 63266 63266
Branches 10052 10052
==========================================
- Hits 42378 42355 -23
- Misses 18449 18518 +69
+ Partials 2439 2393 -46
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
AMoo-Miki
force-pushed
the
2.x-sass-embedded
branch
from
da2d34a to
6e426cb
Compare
* Dart's `sass` uses a lot more memory that `node-sass` which causes failures with CI on Windows when building platform plugins: pagefile size was bumped. Signed-off-by: Miki <miki@amazon.com> (cherry picked from commit f822702) Signed-off-by: Miki <miki@amazon.com>
AMoo-Miki
force-pushed
the
2.x-sass-embedded
branch
from
6e426cb to
110c5bf
Compare
|
CHANGELOG seems to have the entry needed; skipping the workflow for it. |
AMoo-Miki
added
the
Skip-Changelog
AMoo-Miki
changed the title
node-sass with sass-embedded (#5338)node-sass with sass-embedded (#5338)
There was a problem hiding this comment.
@AMoo-Miki Mostly looks good to me. My only hesitation is about the OUI version bump to a non-released version. Is that strictly necessary here?
| "@elastic/eui": "npm:@opensearch-project/oui@1.3.0", | ||
| "@elastic/eui": "npm:@opensearch-project/oui@1.4.0-alpha.2", |
There was a problem hiding this comment.
I don't think we want alpha versions in 2.x branch? In main it makes sense, but we should keep 2.x releasable.
There was a problem hiding this comment.
plus 1, maybe just keep it in main for now
There was a problem hiding this comment.
This is temporary until OUI 1.4 is released.
There was a problem hiding this comment.
Yeah, but it makes the 2.x branch unreleasable, which I thought we wanted to avoid.
| @@ -27,7 +27,7 @@ | |||
| "del": "^6.1.1", | |||
| "getopts": "^2.2.5", | |||
| "pegjs": "0.10.0", | |||
| "sass-loader": "npm:@amoo-miki/sass-loader@10.4.1-node-sass-9.0.0-libsass-3.6.5", | |||
| "sass-loader": "npm:@amoo-miki/sass-loader@10.4.1-node-sass-9.0.0-libsass-3.6.5-with-sass-embedded.rc1", | |||
There was a problem hiding this comment.
This patch is getting increasingly gross 🤢
There was a problem hiding this comment.
since it's an npm fork, can we give it a more friendly name, also are we able to keep sass-loader up to date since the official one is already at 13.3.2 https://github.com/webpack-contrib/sass-loader/tree/v13.3.2, it might cause future troubles in case we need to bump sass-loader version down the road
There was a problem hiding this comment.
@ZilongX we can only use sass-loader@9 with our ancient webpack@4.
packages/osd-optimizer/src/integration_tests/__snapshots__/basic_optimization.test.ts.snap
Show resolved
Hide resolved
Replace node-sass with sass-embedded #5338
sassuses a lot more memory thatnode-sasswhich causes failures with CI on Windows when building platform plugins: pagefile size was bumped.(cherry picked from commit f822702)