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 node-sass with sass (Dart Sass) #692

Merged

Conversation

tnir
Copy link
Collaborator

@tnir tnir commented Jul 15, 2022

What was the end-user problem that led to this PR?

node-sass (LibSass) has been deprecated.

What was your diagnosis of the problem?

sass (Dart Sass) is the active package.

We highly recommend using Dart Sass.
-- https://github.com/webpack-contrib/sass-loader/blob/997f3eb41d86dd00d5fa49c395a1aeb41573108c/README.md

What is your fix for the problem, implemented in this PR?

Replace node-sass with sass.

Why did you choose this fix out of the possible options?

webpack build (npx webpack -d source-map --progress --color) gets slow 2x times (from 1700-2000ms to 3900-4400ms on my local).

fibers package is not recommended for Node.js 16+, so I guess we cannot use it any more.

Fibers is not compatible with Node.js v16.0.0 or later. Unfortunately, v8 commit dacc2fee0f is a breaking change and workarounds are non-trivial. (see introduction to readme).
-- https://github.com/webpack-contrib/sass-loader/blob/997f3eb41d86dd00d5fa49c395a1aeb41573108c/README.md

webpack build performance on Actions

before: https://github.com/rubygems/bundler-site/runs/7360832092?check_suite_focus=true

== External: webpack 5.73.0 compiled with 3 warnings in 4336 ms

after: https://github.com/rubygems/bundler-site/runs/7361858411?check_suite_focus=true

== External: webpack 5.73.0 compiled with 3 warnings in 6209 ms

Signed-off-by: Takuya Noguchi [email protected]

@tnir tnir marked this pull request as ready for review July 16, 2022 04:08
@deivid-rodriguez deivid-rodriguez temporarily deployed to bundler-site-tnir-repla-bwf3yu July 16, 2022 10:16 Inactive
@deivid-rodriguez
Copy link
Member

@tnir Any reason for not creating PRs from the main repository? It's helpful to have review apps by default.

@tnir
Copy link
Collaborator Author

tnir commented Jul 16, 2022

@deivid-rodriguez While I want to do so, in my Gitpod environment I cannot still have the (Write) permission to rubygems/bundler-site repo.

After you invited me to this repo, I already requested it on the following page from https://gitpod.io/new:
github-gitpod-integration

I thinks this is a kind of limitation from Gitpod when the Gitpod/GitHub integration per GitHub Org is not allowed:

$ git config credential.helper
/usr/bin/gp credential-helper
$ git remote -v
origin  https://github.com/rubygems/bundler-site.git (fetch)
origin  https://github.com/rubygems/bundler-site.git (push)
$ git push origin HEAD
remote: Permission to rubygems/bundler-site.git denied to tnir.
fatal: unable to access 'https://github.com/rubygems/bundler-site.git/': The requested URL returned error: 403

A workaround for it would be to use SSH key instead of HTTPS. But due to another limitation of Gitpod, I think I add the SSH key to ~/.ssh every time I restart the workspace (not sure why Gitpod resets ~ on stop/restart of the workspace).

Sorry for your inconvenience to increase your work.

@deivid-rodriguez
Copy link
Member

Don't worry. Is there anything I can do so that you can get this fixed? Should I install gitpod on this repo?

@hsbt
Copy link
Member

hsbt commented Jul 16, 2022

@tnir I approved to request for gitpod now.

@tnir
Copy link
Collaborator Author

tnir commented Jul 16, 2022

I think permission is granted per user. Ask any of rubygems Org owners (I cannot see each member is Org Owner or Org Member) to allow me to use after they confirm how Gitpod integration affects the Org.

The following resources might be helpful:

@tnir
Copy link
Collaborator Author

tnir commented Jul 16, 2022

@hsbt Thanks. It worked just now when I prepared #695 🎉

Copy link
Member

@deivid-rodriguez deivid-rodriguez 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, thanks!

@deivid-rodriguez deivid-rodriguez merged commit 0a0c845 into rubygems:master Jul 16, 2022
@tnir tnir deleted the tnir-replace-node-sass-with-sass branch July 16, 2022 15:03
@tnir tnir self-assigned this Jul 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants