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.

Sign up for GitHub

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

Jump to bottom

Add sassOption implementation to support sass-embedded #64577

Merged
merged 23 commits into from
Jun 19, 2024
Merged

Add sassOption implementation to support sass-embedded #64577

merged 23 commits into from
Jun 19, 2024

Conversation

joostmeijles
Copy link
Contributor

@joostmeijles joostmeijles commented Apr 16, 2024 •
edited
Loading

What?

This PR adds an option to use sass-embedded.

Why?

For large projects sass-embedded improves SCSS compilation time by over 50%.
See also #36160

How?

Added a sassOption implementation to configure the sass-loader which Sass implementation to use.
See also https://www.npmjs.com/package/sass-loader#implementation

I think this a similar approach to what is done for additionalData.

Additional notes

In order to support sass-embedded, sass-loader is upgraded to 12.6.0.

@ijjk ijjk added area: tests Turbopack Related to Turbopack with Next.js. type: next labels Apr 16, 2024
@socket-security Socket Security
Copy link

socket-security bot commented Apr 16, 2024 •
edited
Loading

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/sass-loader@12.6.0 environment +1 110 kB evilebottnawi

🚮 Removed packages: npm/sass-loader@12.4.0

View full report↗︎

@joostmeijles joostmeijles marked this pull request as ready for review April 16, 2024 18:46
@joostmeijles joostmeijles changed the title Add implementation to sassOptions to support sass-embedded Add sassOptions to support sass-embedded Apr 17, 2024
@joostmeijles joostmeijles changed the title Add sassOptions to support sass-embedded Add sassOption implementation to support sass-embedded Apr 17, 2024
@ijjk
Copy link
Member

ijjk commented Apr 17, 2024

Allow CI Workflow Run

  • approve CI run for commit: 8d08562

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

2 similar comments
@ijjk
Copy link
Member

ijjk commented Apr 17, 2024

Allow CI Workflow Run

  • approve CI run for commit: 8d08562

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

@ijjk
Copy link
Member

ijjk commented Apr 17, 2024

Allow CI Workflow Run

  • approve CI run for commit: 8d08562

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

timneutkens
timneutkens reviewed Apr 17, 2024
View reviewed changes
.../basic-module-implementation-embedded-sass/basic-module-implementation-sass-embedded.test.ts Outdated
import { colorToRgb } from 'next-test-utils'

describe('Basic Module Prepend Data Support', () => {
const { next } = nextTestSetup({
Copy link
Member

Choose a reason for hiding this comment

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

In order to be sufficient I think we have to duplicate the entire Sass test suite run to also use sass-embedded. Should be pretty straightforward now that these are in test/e2e/app-dir/scss, could just add a describe.each around the setup to swap the implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added tests, see 652602d

@ijjk ijjk added the tests label Apr 18, 2024
@joostmeijles joostmeijles reopened this Apr 24, 2024
@timneutkens
Copy link
Member

timneutkens commented Apr 24, 2024 •
edited
Loading

TURBOPACK=1 TURBOPACK_BUILD=1 is required to run them (don't use this to build your own apps, it won't fully work yet). We still have to update the test comment to include that for Turbopack build tests 🙂 The others could be flakes yeah.

@joostmeijles
Copy link
Contributor Author

joostmeijles commented Jun 14, 2024 •
edited
Loading

@timneutkens all tests pass except the new flakes. The tests do not fail, but the 2nd run does not complete within the max. minutes the runner has.

See https://github.com/vercel/next.js/actions/runs/9515680178/job/26230449914?pr=64577 and https://github.com/vercel/next.js/actions/runs/9515680178/job/26230451465?pr=64577

What to do here?

timneutkens
timneutkens approved these changes Jun 19, 2024
View reviewed changes
Copy link
Member

@timneutkens timneutkens left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to land now, checked with @ztanner and the failure on the flaky check happens when too many tests are changed

@joostmeijles
Copy link
Contributor Author

Looks good to land now, checked with @ztanner and the failure on the flaky check happens when too many tests are changed

Next step: update branch and merge?

@ztanner ztanner enabled auto-merge (squash) June 19, 2024 14:34
@ztanner ztanner merged commit 50b9966 into vercel:canary Jun 19, 2024
82 checks passed
@joostmeijles joostmeijles deleted the sass-embedded branch June 19, 2024 15:02
@github-actions github-actions bot added the locked label Jul 4, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@timneutkens timneutkens timneutkens approved these changes

@ztanner ztanner ztanner approved these changes

@ijjk ijjk Awaiting requested review from ijjk

@shuding shuding Awaiting requested review from shuding

@huozhi huozhi Awaiting requested review from huozhi

@feedthejim feedthejim Awaiting requested review from feedthejim

@wyattjoh wyattjoh Awaiting requested review from wyattjoh

Assignees
No one assigned
Labels
CI approved Approve running CI for fork locked tests Turbopack Related to Turbopack with Next.js. type: next
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@joostmeijles @ijjk @timneutkens @ztanner @balazsorban44