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

Migrate ETK to new Sass module system #58725

Closed
wants to merge 1 commit into from

Conversation

noahtallen
Copy link
Contributor

@noahtallen noahtallen commented Dec 1, 2021

Changes proposed in this Pull Request

This is the result of running npx sass-migrator module --load-path node_modules "apps/editing-toolkit/**/*.scss"

See this post for more info.

NOTE: this is blocked by WordPress/gutenberg#37044 in Gutenberg

Testing instructions

  • ETK build should successfully complete
  • Smoke test all ETK features on WordPress.com

@noahtallen noahtallen requested a review from a team as a code owner December 1, 2021 22:22
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 1, 2021
@github-actions
Copy link

github-actions bot commented Dec 1, 2021

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

Comment on lines +1 to +2
@use '@wordpress/base-styles/mixins';
@use '@wordpress/base-styles/variables';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we importing mixins and variables if we're only using breakpoints in this file?

@@ -1,4 +1,4 @@
@import '@automattic/onboarding/styles/mixins';
@use '@automattic/onboarding/styles/mixins' as styles-mixins;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this renamed to styles-mixins? I think it's to avoid naming conflicts but there are no other imports in the file 🤔

@import '@automattic/onboarding/styles/variables';
@import '@automattic/onboarding/styles/mixins';
@use '@wordpress/base-styles/mixins';
@use '@wordpress/base-styles/variables' as variables3;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the rationale between renaming styles/mixins to styles-mixins and then, not applying the same logic to base-styles/variables?

I know it's auto-generated by the tool, but still, that intrigues me 😂

Could we rename those variables to something more explicit? There are usages of variables3 inside the code and I have no idea where they come from.

Comment on lines +1 to +3
@use '@wordpress/base-styles/variables';
@use '@wordpress/base-styles/breakpoints';
@use '@automattic/typography/styles/fonts';
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to import this?

@noahtallen
Copy link
Contributor Author

Sorry, I should have marked this as a draft PR! I could have added more context before making it ready for review. But that said, here are some answers:

@zaguiini Great points! This PR is entirely auto-generated by the Sass migration tool. Nearly all of the "rename changes" are just preventing conflicting namespaces. For example, the default namespace for @use 'automattic/foo' would be foo, so if anything else has the same basename of foo, it would conflict. So the migrator tool names the second import foo2 or something like that.

Also noting that variables coming from imports now get prefixed with the new name. Here's a fake example (I made up the variable names and packages):

// Previous syntax with the old module system
@import '@automattic/foo';
@import '@wordpress/foo';

.bar {
  color: $blue; // This variable is defined when I import Automattic's foo.
  margin: $spacingSize; // This variable is defined when I import WordPress's foo.
}
// NEW syntax after running the automatic migrator tool
@use '@automattic/foo';
@use '@wordpress/foo' as foo2;

// Note that the namespace prefixes are automatically added:
.bar {
  color: foo.$blue; // This variable is defined when I import Automattic's foo.
  margin: foo2.$spacingSize; // This variable is defined when I import WordPress's foo.
}

The other points about importing unused files are probably related to an odd Sass architecture. For example, to utilize @wordpress/base-styles/mixins, one has to have the breakpoint variables (like $break-small) defined in the global scope. Previously, you did that by importing @wordpress/base-styles/breakpoints before importing mixins. With the new sass module system, that concept breaks entirely. To fix it, we first have to update the base-styles package to not rely on global variables being defined already. Then we can get rid of the unused import. (I wrote more about that in this core issue, which is blocking this PR: WordPress/gutenberg#37044)

That said, there could easily be a couple of imports that are stale from previous rewrites, so I may double check them.

Though, since my goal is to migrate the entire Calypso repo to the new system, it'd be nice to avoid a lot of manual tweaks. I think there is a part of the migrator tool which can be used to make the namespaces better, so we can try to use that to avoid the weird namespaces like "variables2"

@noahtallen noahtallen marked this pull request as draft December 2, 2021 21:20
@noahtallen
Copy link
Contributor Author

This may need to be done in the future, but worth starting from scratch when it's needed.

@noahtallen noahtallen closed this Jan 25, 2024
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 25, 2024
@sirreal sirreal deleted the try/migrate-etk-sass-module-system branch January 14, 2025 17:58
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.

3 participants