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

Keep the order of execution for require calls #43

Merged
merged 2 commits into from Jul 20, 2016

Conversation

Projects
None yet
4 participants
@dralletje
Copy link
Contributor

commented Feb 22, 2016

This turned out to be a problem when I tried importing RxJs, as it depends on one required file being executed before another. This PR will make sure the order is maintained, so it at least fixes some cases.

I’ve also added a test; I always wanted to say I added ‘tests’ but I really couldn’t think of another edge case to test ;)

Relevant issue on other repo cyclejs/cyclejs#212

@dralletje dralletje force-pushed the dralletje:master branch from 37a4032 to da584d9 Feb 22, 2016

@dralletje

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2016

@Victorystick Anything on this though? As this is necessary for browserify/webpack packages as well :/

@eventualbuddha

This comment has been minimized.

Copy link
Member

commented Mar 3, 2016

So this will cause rollup to add imports in the source order of the requires. That isn't guaranteed to be the actual order, but it is better than it was before so 👍

@dralletje

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2016

@eventualbuddha most packages won't do anything but importing in the top of the file, so for them it's enough. Maybe we should even issue a warning when something is being required in the not-top-level? Because that would hoist it to the top of the file, totally losing context :/

@ulrik59

This comment has been minimized.

Copy link

commented Apr 5, 2016

@dralletje Could you resolve conflicts so it can be merged ? I also need this evolution in order to use Bootstrap. Great work by the way !

Merge remote-tracking branch 'rollup/master'
# Conflicts:
#	test/test.js
@dralletje

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2016

Sorry it took so long, but I rebased, hahaha :-)

@TrySound

This comment has been minimized.

Copy link
Member

commented Jul 20, 2016

Thanks!

@TrySound TrySound closed this Jul 20, 2016

@TrySound TrySound reopened this Jul 20, 2016

@TrySound TrySound merged commit 8c9d77f into rollup:master Jul 20, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@TrySound

This comment has been minimized.

Copy link
Member

commented Jul 20, 2016

Sorry :)

@dralletje

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2016

Hahaha, thanks :D!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.