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

Replace deprecated node-sass with sass #161813

Merged

Conversation

patrykkopycinski
Copy link
Contributor

@patrykkopycinski patrykkopycinski commented Jul 12, 2023

@patrykkopycinski patrykkopycinski marked this pull request as ready for review July 13, 2023 10:37
@patrykkopycinski patrykkopycinski requested review from a team as code owners July 13, 2023 10:37
@patrykkopycinski patrykkopycinski self-assigned this Jul 13, 2023
@patrykkopycinski patrykkopycinski added the release_note:skip Skip the PR/issue when compiling release notes label Jul 13, 2023
@delanni
Copy link
Contributor

delanni commented Jul 13, 2023

Also, removing this would save us from having to build and serve node-sass binaries for the supported platforms. cc: @elastic/kibana-operations

Copy link
Member

@jbudz jbudz left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this! Have we considered using sass-embedded instead of sass?

It looks like there's almost a 3x slowdown in performance between node-sass and sass

Copy link
Member

@mistic mistic left a comment

Choose a reason for hiding this comment

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

I agree with @jbudz here. We should experiment to use sass-embedded instead of sass. It offers the same upsides for us ( 1- move ahead from a legacy thing ; 2 - include prebuilds for every important architecture) while it should preserve the previous performance on builds

@patrykkopycinski patrykkopycinski force-pushed the chore/replace-node-sass branch from ad0c75f to 9724bd9 Compare July 13, 2023 14:30
@patrykkopycinski
Copy link
Contributor Author

@jbudz @mistic @delanni unfortunately we are not able to use sass-embedded, because sass-loader in current version doesn't support sass-embedded and we cannot bump the loader, becuase from ^11.0.0 it requires webpack@5 🤷‍♂️

@mistic
Copy link
Member

mistic commented Jul 15, 2023

I'm just worried this is adding a performance penalty to a such important job step like Build Kibana Distribution and Plugins. In some cases this is adding 7 minutes to that job (from 13 min to 20) which will impact have impact on other jobs like FTR (and possible the rest of the ci). @jbudz what are your thoughts here? I'm divided on move on and deal with the consequences until we upgrade to webpack 5 (and can use the embedded) or hold this off until we upgrade webpack and then use this PR with the sass embedded instead of this one.

@jbudz
Copy link
Member

jbudz commented Jul 17, 2023

I'm just worried this is adding a performance penalty to a such important job step like Build Kibana Distribution and Plugins. In some cases this is adding 7 minutes to that job (from 13 min to 20) which will impact have impact on other jobs like FTR (and possible the rest of the ci). @jbudz what are your thoughts here? I'm divided on move on and deal with the consequences until we upgrade to webpack 5 (and can use the embedded) or hold this off until we upgrade webpack and then use this PR with the sass embedded instead of this one.

node-sass is still under maintenance. If there's a must-have feature I think we can make the case for upgrading, but in isolation I don't see a benefit over the significant time increase in plugin builds. Overall it looks like +4 minutes on CI, and a matching (and less parallel) experience for development.

It is good to know we can upgrade now though, if needed. The previous attempt didn't work out #61722.

@mistic
Copy link
Member

mistic commented Jul 18, 2023

I agree with Jon here. I think this is good work but the benefits of doing it now without upgrading the webpack to v5 so we can use the embedded version don't pay the costs (CI overhead) IMHO. It's probably better to hold off this PR until that moment and then we can move on with it

@patrykkopycinski
Copy link
Contributor Author

woooooah, great news @mistic!

@mistic
Copy link
Member

mistic commented Dec 7, 2023

@patrykkopycinski can we revive this? v10.5.0 was released and I think we can now use it instead of the node_module patch solution https://www.npmjs.com/package/sass-loader/v/10.5.0

Copy link
Member

@mistic mistic left a comment

Choose a reason for hiding this comment

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

lgtm

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
canvas 1013.7KB 1013.7KB -2.0B
cloudSecurityPosture 451.7KB 451.7KB +12.0B
controls 200.2KB 200.2KB -6.0B
dashboard 376.4KB 376.4KB +4.0B
dataViewManagement 120.0KB 120.0KB -2.0B
dataVisualizer 617.6KB 617.6KB +10.0B
discover 592.2KB 592.2KB +14.0B
enterpriseSearch 2.7MB 2.7MB -52.0B
eventAnnotationListing 196.8KB 196.8KB +16.0B
filesManagement 89.9KB 89.9KB -4.0B
graph 387.3KB 387.3KB +8.0B
home 178.6KB 178.6KB -14.0B
indexManagement 585.1KB 585.2KB +6.0B
infra 1.9MB 1.9MB -4.0B
ingestPipelines 360.8KB 360.8KB -4.0B
kibanaOverview 46.6KB 46.6KB -4.0B
kibanaReact 214.5KB 214.7KB +194.0B
lens 1.4MB 1.4MB +4.0B
logExplorer 981.7KB 981.7KB +10.0B
management 75.0KB 75.0KB -4.0B
maps 2.9MB 2.9MB +36.0B
metricsDataAccess 85.2KB 85.2KB -4.0B
ml 3.6MB 3.6MB +28.0B
monitoring 462.2KB 462.3KB +66.0B
observabilityShared 36.3KB 36.3KB -4.0B
osquery 1.0MB 1.0MB -4.0B
presentationUtil 82.1KB 82.1KB +10.0B
security 575.2KB 575.2KB +16.0B
securitySolution 11.3MB 11.3MB +2.0B
securitySolutionEss 42.7KB 42.7KB -4.0B
securitySolutionServerless 361.1KB 361.1KB -4.0B
spaces 174.9KB 174.9KB -4.0B
stackAlerts 80.6KB 80.6KB +10.0B
unifiedDocViewer 58.4KB 58.4KB +6.0B
unifiedSearch 213.7KB 213.7KB +8.0B
visDefaultEditor 141.0KB 140.9KB -14.0B
visTypeTimelion 38.8KB 38.8KB -4.0B
visTypeVega 1.8MB 1.8MB -4.0B
visTypeVislib 339.7KB 339.7KB +40.0B
visualizations 266.9KB 266.9KB -28.0B
total +330.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
core 375.5KB 375.6KB +76.0B
embeddable 78.2KB 78.2KB +20.0B
esUiShared 155.8KB 155.8KB -4.0B
globalSearchBar 26.3KB 26.3KB -6.0B
kbnUiSharedDeps-npmDll 6.2MB 6.2MB +280.0B
kibanaOverview 15.2KB 15.1KB -24.0B
kibanaReact 41.8KB 41.8KB -6.0B
osquery 51.2KB 51.2KB +10.0B
searchprofiler 18.7KB 18.8KB +18.0B
total +364.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @patrykkopycinski

@patrykkopycinski patrykkopycinski merged commit 87d5d6b into elastic:main Dec 21, 2023
37 checks passed
@patrykkopycinski patrykkopycinski deleted the chore/replace-node-sass branch December 21, 2023 11:52
@kibanamachine kibanamachine added v8.13.0 backport:skip This commit does not require backporting labels Dec 21, 2023
jbudz added a commit that referenced this pull request Dec 21, 2023
@jbudz
Copy link
Member

jbudz commented Dec 21, 2023

@patrykkopycinski This was reverted with 3def20e. See https://buildkite.com/elastic/kibana-artifacts-snapshot/builds/3735 for build failures, and https://buildkite.com/elastic/kibana-pull-request/builds/184823#018c8d10-8206-4937-9122-e13d5616f81a for the followup failures after the output style was changed.

@jbudz jbudz added the reverted label Dec 21, 2023
@patrykkopycinski patrykkopycinski restored the chore/replace-node-sass branch December 23, 2023 00:26
@patrykkopycinski
Copy link
Contributor Author

Sorry @jbudz, fixed version #173942
Maybe we could add "Build Kibana artifacts" pipeline to the PR check to avoid that in the future?

jbudz added a commit that referenced this pull request Dec 28, 2023
## Summary

The previous PR #161813 was
reverted due to the broken webpack config

eef1afc

---------

Co-authored-by: Tiago Costa <tiago.costa@elastic.co>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Jon <jon@elastic.co>
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Dec 28, 2023
## Summary

The previous PR elastic#161813 was
reverted due to the broken webpack config

elastic@eef1afc

---------

Co-authored-by: Tiago Costa <tiago.costa@elastic.co>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Jon <jon@elastic.co>
(cherry picked from commit d4be2a3)
jbudz added a commit to jbudz/kibana that referenced this pull request Jan 2, 2024
The previous PR elastic#161813 was
reverted due to the broken webpack config

elastic@eef1afc

---------

Co-authored-by: Tiago Costa <tiago.costa@elastic.co>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Jon <jon@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes reverted v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants