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

Predictable imports order #138

Merged
merged 7 commits into from
May 22, 2017
Merged

Predictable imports order #138

merged 7 commits into from
May 22, 2017

Conversation

sullenor
Copy link
Member

@sullenor sullenor commented Apr 1, 2017

A small attempt of mine to make a proper dependency resolution for the case #131:

.a {
  composes: c from './c.css';
}

.b {
  composes: b from './b.css';
  composes: c from './c.css';
}

@codecov-io
Copy link

codecov-io commented Apr 4, 2017

Codecov Report

Merging #138 into master will increase coverage by 1.99%.
The diff coverage is 94.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #138      +/-   ##
==========================================
+ Coverage   91.56%   93.56%   +1.99%     
==========================================
  Files           2        4       +2     
  Lines          83      233     +150     
  Branches       20       46      +26     
==========================================
+ Hits           76      218     +142     
- Misses          7       15       +8
Impacted Files Coverage Δ
src/topologicalSort.js 100% <100%> (ø)
src/index.js 92.3% <91.66%> (-7.7%) ⬇️
lib/topologicalSort.js 97.05% <0%> (ø)
lib/index.js 91.2% <0%> (+5.79%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2c40a2...ed63ddc. Read the comment docs.

@sullenor
Copy link
Member Author

sullenor commented Apr 4, 2017

@geelen @markdalgleish @sokra @joshwnj in case you have some time, please, take a look into it. Since this module is commonly used, the changes can be a bit breaking for the existing projects :)

thanks

}

const processor = postcss.plugin('modules-extract-imports', function (options = {}) {
const failOnWrongOrder = options.failOnWrongOrder;
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure about the option's name failOnWrongOrder. In case you have a better variant — tell me.

The purpose of it is to warn a user if he made a dependency order which isn't possible to resolve properly:

.a {
  composes: aa from './aa.css';
  composes: bb from './bb.css';
}

.b {
  composes: bb from './bb.css';
  composes: aa from './aa.css';
}

@sullenor sullenor requested review from sokra and geelen and removed request for sokra and geelen April 6, 2017 12:15
@sullenor
Copy link
Member Author

Up!

@tivac
Copy link

tivac commented Apr 25, 2017

Not that it matters but a huge 👍 from me, adding a dependency graph to imports in modular-css was super valuable.

@sullenor
Copy link
Member Author

sullenor commented May 1, 2017

I hope that smone can ship it since I don't any permission to publish the package

@geelen
Copy link
Member

geelen commented May 22, 2017

This looks fine to me. I am a little worried that it might very subtly break the resulting CSS for some existing users, so I think it might make sense to publish this under a dist-tag to give it some real-world testing before turning it on for everyone?

@sullenor
Copy link
Member Author

dist-tag would be fine

@sullenor sullenor merged commit 7f4028c into master May 22, 2017
@sullenor sullenor deleted the predictable-imports-order branch May 22, 2017 06:39
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.

None yet

4 participants