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

Nested import ordering #236

Merged
merged 1 commit into from
Nov 3, 2016
Merged

Nested import ordering #236

merged 1 commit into from
Nov 3, 2016

Conversation

RyanZim
Copy link
Collaborator

@RyanZim RyanZim commented Nov 2, 2016

Had to edit a test that depended on the old, incorrect behavior.

Fixes #168 🎉

@RyanZim
Copy link
Collaborator Author

RyanZim commented Nov 3, 2016

This has a few problems, do not merge.

Had to edit a test that depended on the old, incorrect behavior
@RyanZim
Copy link
Collaborator Author

RyanZim commented Nov 3, 2016

Fixed. @MoOx ?

@@ -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

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

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

Choose a reason for hiding this comment

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

Clear enough! Thanks!

@MoOx MoOx merged commit 2f93068 into postcss:master Nov 3, 2016
@MoOx
Copy link
Contributor

MoOx commented Nov 3, 2016

@RyanZim what is your npm nickname?

@MoOx
Copy link
Contributor

MoOx commented Nov 3, 2016

You know have the full rights for this repo.

To cut a release you will need to:

  • be logged on npm from your terminal
  • update changelog to add a new section + bump version number in package.json (like explained here https://github.com/MoOx/npmpub#requirements, you don't need to install it, it's already a dev deps of this project)
  • commit your changelog add + package.json update with a message being the version number for clarity
  • run "npm run release". This should run test with a fresh node_modules (to ensure everything will work correctly from a fresh install), then will release a new version to npm and will cut a github release using notes from the changelog

If you are not confortable doing this, I can do this with you if you want, or I can even do it alone, it will take me a minute.

@RyanZim RyanZim deleted the nested-import branch November 3, 2016 15:14
@RyanZim
Copy link
Collaborator Author

RyanZim commented Nov 3, 2016

@MoOx ryanzim

https://www.npmjs.com/~ryanzim

@MoOx
Copy link
Contributor

MoOx commented Nov 3, 2016

image

@RyanZim
Copy link
Collaborator Author

RyanZim commented Nov 3, 2016

@MoOx Couple of questions:

  • master requires the status check continuous-integration/appveyor/branch. Isn't that incorrect?
  • Should this change be published immediately, or should we add a few other things first? CC: @ai

@MoOx
Copy link
Contributor

MoOx commented Nov 3, 2016

I think when there are fork, continuous-integration/appveyor/branch is incorrectly not working as expected. No big deal imo.
You can release this asap as a patch. Do not hesitate to cut 3 releases in a day if you feel the need :) As soon as you respect semver (tldr: breaking change: major, new feature: minor, fix: patch).

@RyanZim
Copy link
Collaborator Author

RyanZim commented Nov 3, 2016

I think when there are fork, continuous-integration/appveyor/branch is incorrectly not working as expected.

Branch only runs on internal PRs.

@RyanZim RyanZim mentioned this pull request Nov 3, 2016
@RyanZim
Copy link
Collaborator Author

RyanZim commented Nov 3, 2016

@MoOx I would like to do releases in the future; but can you ship this today? I don't have time ATM.

@MoOx
Copy link
Contributor

MoOx commented Nov 3, 2016

I just released 8.1.3

@RyanZim
Copy link
Collaborator Author

RyanZim commented Nov 3, 2016

Thanks!

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.

2 participants