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

Fix race condition where last format transform overrides previous ones #1149

Merged
merged 3 commits into from Nov 25, 2019

Conversation

@m-mujica
Copy link
Contributor

m-mujica commented Nov 17, 2019

The transform function resolved by transformInput was handling
the bundle concatenation to the event loop too early, causing future
transforms (sharing the same graph) to reset the activeSourceKeys
object -where the result of transpiled graphs are kept- before the bundle
was concatenated.

This meant the last transform would override all of the previous ones,
instead of doing return Promise.resolve().then(concatenateBundle), this
commit creates a new promise that runs concatenateBundle right away
instead of waiting for the next tick (and opening the window from future
transforms to mutate the graph).

Closes #1139

m-mujica added 2 commits Nov 17, 2019
Importing individual tests files in test/test would sometimes cause
the whole test suite not to run if any of the tests failed; also
sometimes the test suite finishes early for no apparent reason.

This approach is meant to fix these issues. Providing glob patterns
to the CLI is not practical in this case since the test folders holds
code that is not meant to be run my mocha directly.
The transform function resolved by `transformInput` was handling
the bundle concatenation to the event loop too early, causing future
transforms (sharing the same graph) to reset the activeSourceKeys
object where the result of transpiled graphs are kept, before the bundle
was concatenated.

This meant, the last transform would override all of the previous ones,
instead of doing `return Promise.resolve().then(concatenateBundle)`, this
commit creates a new promise that runs "concatenateBundle" right away
instead of waiting for the next tick (and opening the window from future
transforms to mutate the graph).

Closes #1139
@m-mujica m-mujica requested a review from matthewp Nov 17, 2019
@m-mujica

This comment has been minimized.

Copy link
Contributor Author

m-mujica commented Nov 17, 2019

tenor

I did not consider it would break the test coverage scripts
@stealjs stealjs deleted a comment from coveralls Nov 17, 2019
@matthewp

This comment has been minimized.

Copy link
Member

matthewp commented Nov 24, 2019

lgtm, awesome work

@m-mujica m-mujica merged commit ac5e46e into master Nov 25, 2019
5 checks passed
5 checks passed
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.7%) to 94.049%
Details
@m-mujica m-mujica deleted the fix-exports branch Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.