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 away from @import #2320

Open
adamstankiewicz opened this issue May 20, 2023 · 3 comments
Open

Migrate away from @import #2320

adamstankiewicz opened this issue May 20, 2023 · 3 comments
Labels
awaiting prioritization engineering Engineering maintenance Routine upkeep necessary for the health of the platform

Comments

@adamstankiewicz
Copy link
Member

adamstankiewicz commented May 20, 2023

See the "Heads up!" section here: https://sass-lang.com/documentation/at-rules/import

The Sass team discourages the continued use of the @import rule. Sass will gradually phase it out over the next few years, and eventually remove it from the language entirely. Prefer the @use rule instead.

Paragon heavily uses @import, as does its consuming MFEs to import styles from other packages. SCSS recommends the use of @use, so this issue is to track the work to migrate away from @import in favor of @use.

The above linked docs also suggest using a migrator CLI: https://sass-lang.com/documentation/cli/migrator

@adamstankiewicz adamstankiewicz added awaiting prioritization maintenance Routine upkeep necessary for the health of the platform engineering Engineering labels May 20, 2023
@bradenmacdonald
Copy link
Contributor

@adamstankiewicz
Copy link
Member Author

adamstankiewicz commented Apr 25, 2024

Related? openedx/frontend-app-learner-dashboard#325

@bradenmacdonald Unlikely that this Paragon issue is related to the above linked learner-dashboard issue. I believe the issue happening in learner-dashboard is because specific SCSS files are re-importing Paragon's SCSS even though it's imported in the SCSS entry point (App.scss). See this specific SCSS file as an example (there are several others).

The likely reason the specific SCSS files are re-importing Paragon styles is due to Paragon's SCSS variables needing to be defined prior to using them within the file. The simplest way to do this is simply importing all of Paragon's SCSS in each individual SCSS file (current state in this MFE), however, the more correct/accepted approach to prevent duplicate Paragon CSS would be import the specific SCSS files within the SCSS entry point (App.scss).

For example:

@bradenmacdonald
Copy link
Contributor

@adamstankiewicz

the more correct/accepted approach to prevent duplicate Paragon CSS would be import the specific SCSS files within the SCSS entry point (App.scss)

Yes, I tried to mention that same thing in the suggested fixes for that issue. However, that also results in larger bundle size as it results in one CSS bundle for the whole MFE, whereas if you have multiple CSS entry points (without duplication) you can sometimes get smaller initial page load times due to code splitting where some CSS and JS is loaded only when needed.


However, while working on the Vite frontend shell demo I discovered there is a bigger problem with how Paragon structures its SCSS exports: namely, it doesn't provide a clear "rule-free" export of all the Paragon SCSS.

For example, in that demo, the "Shell" app is already providing the Paragon SCSS so for the MFE I want its SCSS files to be able to import all the Paragon mixins, functions, variables, etc. and emit rules that build on them, but without emitting the base paragon/bootstrap rules into the build. If Paragon's SCSS was structured in a way that separates variables from rules, then the MFE could just import @openedx/paragon/without-rules (or something) and it wouldn't matter if that was @use or @imported in every file.

But what I found is that to achieve that with Paragon as it exists now, the MFE has to do something like this:

@import "@edx/brand/paragon/variables";
@import "@openedx/paragon/scss/core/_functions";
@import "@openedx/paragon/scss/core/_variables";
@import "~bootstrap/scss/mixins";
@import "@openedx/paragon/src/Form/_variables.scss"; // for $input-box-shadow.
@import "@openedx/paragon/src/Card/_variables.scss"; // for $card-image-vertical-max-height
@import "@openedx/paragon/src/Alert/_variables.scss"; // for $alert-border-radius

And since that's using a lot of "private" files, it kind of encourages people to just @import the whole Paragon SCSS, making that type of duplicate rules bug much easier to accidentally do.

I also found that @use instead of @import isn't a magic bullet for solving the duplicate rules issue; if you have a single entry point into your SCSS (as the MFEs you pointed to do), then yeah it works great. But if you have multiple entry points, it doesn't prevent any duplicate rules.

I guess that is not directly related to the issue at hand, but it's something I hope people keep in mind when working on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting prioritization engineering Engineering maintenance Routine upkeep necessary for the health of the platform
Projects
Status: Backlog
Status: Backlog
Development

No branches or pull requests

2 participants