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

Steal should de-duplicate less imports. #1394

Closed
1 of 5 tasks
mjstahl opened this issue May 10, 2018 · 7 comments
Closed
1 of 5 tasks

Steal should de-duplicate less imports. #1394

mjstahl opened this issue May 10, 2018 · 7 comments

Comments

@mjstahl
Copy link
Contributor

mjstahl commented May 10, 2018

How often can you reproduce it?

  • Always
  • Sometimes
  • Rarely
  • Unable
  • I didn’t try

Description:
Our less plugin for steal uses the less engine to compile less to css. The problem occurs when you mix global styles with component-specific or modlet styles that import that global style.

When importing the same less file (like a global style) in multiple component less files, the rules of the global file are duplicated. I imagine this is due to steal and the less engine treating each less file in isolation.

The resolution for this is to have a global style sheet that imports all other style sheets. This creates a single entry point for the steal/less to create CSS sans any duplication. The problem with /this/ fix is that it breaks the idea of isolated self-contained modlets/components. The other problem with this approach is probably bundle sizes as we are including more styles than required.

Steps to reproduce:

  1. Create an application and create one global style file. For example a buttons.less file that includes the all the applications styles for button elements.
  2. Create two modlets, and in the less files for those modlets do @import path/to/buttons.less.

Expected results:
The styles from buttons.less would only be included once.

Actual results:
Each modlet style sheet includes the styles from buttons.less.

Environment:

Software Version
Steal version 1.11.0
Steal-tools version 1.9.1
node -v v8.11.1
npm -v 5.6.0
Browser any
Operating system macOS 10.13.4
@mjstahl
Copy link
Contributor Author

mjstahl commented May 10, 2018

A proposed solution to this that might minimize the effort would be build a "global style sheet" in memory and use less to transpile it to CSS.

@matthewp
Copy link
Member

From a technical perspective it's somewhat difficult to know what is "global". You need to inject this stylesheet just before the main loads (for stuff in the main bundle) or just before the bundle loads for a progressively loaded package. That being said anything is achievable. I think having an optional way to do this in steal-less would be possible.

@matthewp
Copy link
Member

matthewp commented Jun 1, 2018

This was discussed on a recent live stream (6:27).

Proposed Solution

For each bundle (including the main bundle), create a special in-memory less module. For each less module that is imported in the tree, recompile the special module appending the imported module.

Technical hurdles

Knowing what tree a particular module belongs to is a little bit tricky. Some code exists within done-ssr that attempts to do something similar, so we should reuse that if it will be helpful. This might necessitate a new feature in steal first.

@matthewp matthewp changed the title Steal should de-duplicate less/scss/sass imports. Steal should de-duplicate less imports. Jun 1, 2018
@JaneOri
Copy link
Contributor

JaneOri commented Jun 1, 2018

Possibly relevant test case for multiple css import causing problems:
stealjs/steal-css#16 (comment)

Technically any shared less file - like a buttons.less - should only contain variables, mixins, functions, and anything else that doesn't produce output on its own. Common less files should not contain anything that produces output on its own.

The problem is still possibly important to solve for another case though - when you have a component that's reused and imported multiple times. Ideally the less for that component should only be inserted once.

@jeroencornelissen
Copy link

jeroencornelissen commented Jun 4, 2018

We've noticed that too, variables.less or buttons.less get loaded multiple times. Is this ticket for a production build or develop? This could improve our loading times when developing.
For production build we've setup cleanCSSOptions in steal-tools build config.

@matthewp
Copy link
Member

@jeroencornelissen This would be for both.

@matthewp
Copy link
Member

Issue moved to stealjs/steal-less #72 via ZenHub

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

No branches or pull requests

4 participants