Skip to content

Nested import ordering #236

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 3, 2016
Merged

Nested import ordering #236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Fix #168: Nested import ordering 
Had to edit a test that depended on the old, incorrect behavior
  • Loading branch information
@RyanZim
RyanZim committed Nov 3, 2016
commit 9f9b10f2b5f737a0dde6dd23efeee0dc7936a905
3 changes: 2 additions & 1 deletion index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ var joinMedia = require("./lib/join-media")
var resolveId = require("./lib/resolve-id")
var loadContent = require("./lib/load-content")
var parseStatements = require("./lib/parse-statements")
var promiseEach = require("promise-each")

function AtImport(options) {
options = assign({
Expand Down Expand Up @@ -163,7 +164,7 @@ function parseStyles(
) {
var statements = parseStatements(result, styles)

return Promise.all(statements.map(function(stmt) {
return Promise.resolve(statements).then(promiseEach(function(stmt) {
Copy link
Contributor

@MoOx MoOx Nov 3, 2016

Choose a reason for hiding this comment

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

what is the difference between promise each and Promise.all? Cause from the doc I can see

The array of values maintains the order of the original iterable object, not the order that the promises were resolved in.

Copy link
Collaborator Author

@RyanZim RyanZim Nov 3, 2016

Choose a reason for hiding this comment

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

@MoOx I should have explained these changes better.

map runs the function down to return resolveImportId(). This starts a promise chain. map then goes on to the next item in statements, without waiting for resolveImportId to resolve. This causes concurrency. Depending on the order that the files are loaded, state is modified incorrectly. If foo-second.css loads first, bar.css is added to state.importedFiles. When foo-first.css is loaded, bar.css appears to be already loaded, however, it is in the incorrect order in the bundle.

promiseEach waits for resolveImportId to resolve before going to the next item in statements. promise-each is a standalone port of http://bluebirdjs.com/docs/api/promise.each.html.

This is a performance cut; but this is the quickest way to fix this. If someone can figure out a better deduping system that does not require serialized loading, great; I can't ATM.

Hope this is clear.

Copy link
Contributor

@MoOx MoOx Nov 3, 2016

Choose a reason for hiding this comment

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

Clear enough! Thanks!

stmt.media = joinMedia(media, stmt.media || [])

// skip protocol base uri (protocol://url) or protocol-relative
Expand Down
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
"pkg-resolve": "^0.1.7",
"postcss": "^5.0.14",
"postcss-value-parser": "^3.2.3",
"promise-each": "^2.2.0",
"read-cache": "^1.0.0",
"resolve": "^1.1.7"
},
Expand Down
22 changes: 4 additions & 18 deletions test/order.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,27 +3,13 @@ import compareFixtures from "./helpers/compare-fixtures"

test(`should order nested imports correctly`, t => {
var first = true
var resolve = require("resolve")
var path = require("path")

return compareFixtures(t, "order", {
path: "fixtures/imports",
resolve: (id, base, opts) => {
var resolveOpts = {
basedir: base,
moduleDirectory: [],
paths: opts.path,
extensions: [ ".css" ],
}

return new Promise(function(res, rej) {
var doResolve = () => {
resolve(id, resolveOpts, function(err, path) {
if (err) {
return rej(err)
}
res(path)
})
}
resolve: (id) => {
return new Promise(function(res) {
var doResolve = () => res(path.resolve("fixtures/imports", id))

if (first) {
// Delay the first import so the second gets loaded first
Expand Down
2 changes: 1 addition & 1 deletion test/plugins.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ test("should apply plugins to root", t => {
})
.then(() => {
t.deepEqual(atRules, [ "import" ])
t.deepEqual(rules, [ "bar", "foo" ])
t.deepEqual(rules, [ "foo", "bar" ])
})
})

Expand Down