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

Replace "collections/deque" with "queue" #25

Closed
wants to merge 1 commit into from

Conversation

Bazze
Copy link

@Bazze Bazze commented Oct 17, 2016

The "collections" package has issues with array shims causing other dependencies to break when least expected. This has been discussed in multiple issues:

Another solution to this could also be to depend on "[email protected]" instead, which is a version without shims.

All tests pass with this change, so assuming they cover the queue part this change should be fine.

The "collections" package has issues with array shims causing other
dependencies to break when least expected. This has been discussed
in multiple issues:

 - montagejs/collections#162
 - montagejs/collections#95
 - balderdashy/sails#2524

Another solution to this could be to depend on "[email protected]"
instead which is a version without shims.
@victorquinn
Copy link
Owner

victorquinn commented Oct 18, 2016

Hey @Bazze I totally appreciate this fix but I had actually already noticed and started on a solution to this problem.

Yesterday, I went through and replaced all references in lib/connection.js to collections/deque to use the Immutable.js List. I was going to finish by doing the same in client.js today.

I'd rather keep things consistent so I'd like to use the same library in both places and not use queue in lib/client.js and Immutable.List in lib/connection.js so unfortunately I'm not going to merge this.

Thanks again for the work and I'm sorry again that I won't merge this.

@Bazze
Copy link
Author

Bazze commented Oct 19, 2016

Great, sounds like an even better solution!

@Bazze
Copy link
Author

Bazze commented Oct 19, 2016

Btw, when do you think you'll push a new version to npm that includes the change? :)

@victorquinn
Copy link
Owner

Yup, pushed just now to npm as 0.2.13!

@Bazze Bazze deleted the replace-queue-lib branch March 9, 2017 21:17
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