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 to use the Sass module system #5220

Open
jathak opened this issue Nov 1, 2019 · 9 comments

Comments

@jathak
Copy link

@jathak jathak commented Nov 1, 2019

As discussed offline with @abhiomkar and @aprigogin, we'd like to migrate the stylesheets in this repo to use the Sass module system.

I'm working on preparing a script that uses the Sass migrator to handle most of this migration, but I have a question about how to handle member prefixes.

Right now, most variables, mixins, and functions are manually namespaced with mdc-<component>-. Since the module system adds automatic namespacing, it makes sense to remove these manual prefixes (which the migrator can handle automatically).

However, there are two cases where this pattern doesn't quite hold:

Note that however we choose to handle namespacing during this migration, we'll add import-only files that forward the members with their original, manually-namespaced names. When downstream users migrate, the migrator will be able to detect this and automatically change these names when necessary.

@abhiomkar

This comment has been minimized.

Copy link
Contributor

@abhiomkar abhiomkar commented Nov 1, 2019

Thanks @jathak for the update!

  • We can leave color palette names as-is for import path by forwarding with material-color-* prefix.
  • This is interesting case, what options do we have? I wish @forward rule supports forwarding per mixin or variable name instead of file name.

Here is what I'm thinking:

Option 1

// rtl/_base.scss (Sass module)
@mixin rtl() {...}

// rtl/_mixins.scss (Sass module)
@mixin reflexive-box() {...}
@forward "base"

// rtl/_mixins.import.scss (Import path)
@forward "mixins" as mdc-rtl-*
@forward "base" as mdc-*

The first forward above might include mdc-rtl-rtl() mixin, but I think it should be fine. WDYT?

Option 2

Rename those mixins, but this requires changing all the places where these mixins were used. Not preferable.

@jathak

This comment has been minimized.

Copy link
Author

@jathak jathak commented Nov 4, 2019

For option 1, we wouldn't actually have to split it into a separate file, we could just do:

// rtl/_mixins.import.scss
@forward "mixins" as mdc-rtl-* hide mdc-rtl-rtl;
@forward "mixins" as mdc-* show mdc-rtl;

That should have the same effect as Option 1, but without requiring any new files. Would that work for you?

@abhiomkar

This comment has been minimized.

Copy link
Contributor

@abhiomkar abhiomkar commented Nov 4, 2019

I think that looks even better. Thanks! :)

@jathak

This comment has been minimized.

Copy link
Author

@jathak jathak commented Nov 5, 2019

One other non-standard namespace I found: for mdc-ripple, most members use a mdc-ripple prefix as expected, but the two functions in _functions.scss are prefixed with mdc-states instead. How should these be migrated?

@abhiomkar

This comment has been minimized.

Copy link
Contributor

@abhiomkar abhiomkar commented Nov 5, 2019

Can we do the same as above?

@jathak

This comment has been minimized.

Copy link
Author

@jathak jathak commented Nov 5, 2019

I think the three options here are:

  1. Remove the full prefixes from both mdc-ripple- and mdc-states-.
  2. Remove the full prefix from mdc-ripple- but only remove mdc- from mdc-states-.
  3. Remove only mdc- from both.

All three are possible, but it looks like there's also an mdc-states mixin, so for (1), that would still need to be handled like the others, and for both (1) and (2) we'd need to migrate a lot of this component manually since the migrator only supports one prefix per file. We'd also end up with fairly complex show/hide clauses to preserve the original prefixes in the import-only files. (3) would be the easiest for the migrator, but you would end up with longer names. Which would you prefer?

@abhiomkar

This comment has been minimized.

Copy link
Contributor

@abhiomkar abhiomkar commented Nov 5, 2019

Thanks for providing these options! I prefer an option which is backward compatible. I'm also open to renaming any of these mixins if you think the migration is getting super complicated, but I prefer it as last resort.

Here is what I'm proposing -

  • mdc-ripple
    • name in module system: ripple
    • import path: forwarded with mdc-*
  • mdc-ripple-radius-bounded
    • name in module system: radius-bounded
    • import path: forwarded with mdc-ripple-*
  • mdc-states
    • name in module system: states
    • import path: forwarded with mdc-*
  • mdc-states-hover-opacity
    • name in module system: states-hover-opacity
    • import path: forwarded with mdc-*

This makes the import path fully backward compatible. Please let me know your thoughts.

@jathak

This comment has been minimized.

Copy link
Author

@jathak jathak commented Nov 5, 2019

Yeah, that should work (it's mostly my option 2). I don't think there's any member named exactly mdc-ripple, so that shouldn't be an issue, and there's no real difference between mdc-states and mdc-states-something-longer from the migrator's perspective when it's only removing the mdc- prefix. I'll try to set up a script to migrate mdc-ripple this way and let you know if I run into issues.

@jathak

This comment has been minimized.

Copy link
Author

@jathak jathak commented Nov 18, 2019

An update on this: we realized that the module system as-is did not allow variables behind a @forward to be configured through an @import (which would mean migrating a library to the module system would prevent downstream users still on @import from configuring it). Since that blocks this migration, we've proposed a language change to support this and are working on the implementation now. See sass/sass#2772 for more details on this.

Another thing I realized is running the migrator in its current state on material-components-web could break downstream users that implicitly depended on files they didn't directly import (e.g. depending on a variable from mdc-theme while only directly importing mdc-button). Do you know how common this is / do we need to make sure we don't break this? The fix would be to have the migrator forward all dependencies that were previously available implicitly from an import-only file, which is feasible, but would take some time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.