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

Hotfix release needed - Issue with latest rxjs release candidate #178

Closed
henryruhs opened this issue Nov 7, 2016 · 9 comments
Closed

Hotfix release needed - Issue with latest rxjs release candidate #178

henryruhs opened this issue Nov 7, 2016 · 9 comments

Comments

@henryruhs
Copy link

henryruhs commented Nov 7, 2016

Hello,

there is an issue with "[email protected]" that was released a day ago. This is going to break all IED released since 2.0.0 until 2.3.3. I suggest to release a hotfix as soon as possible. It is not a good idea to use a beta in combination with ^ in the version number - I hope someone learns a lesson from that :-)

Error:
Cannot find module 'rxjs/operator/distinctKey'

Broken build:
https://travis-ci.org/redaxmedia/redaxscript-download-sync/builds/173742057

Temporary solution is to install the earlier version of rxjs:

- npm install --global ied
- npm install --global [email protected]
- ied install

Fixed build:
https://travis-ci.org/redaxmedia/redaxscript-download-sync/builds/173776343

Solution for package.json:

"rxjs": "[email protected]"
  • Version: latest
  • Platform: Travis CI / Debian
  • Subsystem: NodeJS 6.9.1
@henryruhs henryruhs changed the title Emercancy release needed - Issue with latest rxjs release candidate Hotfix release needed - Issue with latest rxjs release candidate Nov 7, 2016
@alexanderGugel
Copy link
Owner

Awesome! Thanks.

Fix released as [email protected].

@henryruhs
Copy link
Author

This does not fix anything, because you are using ^ again... lol

@alexanderGugel
Copy link
Owner

alexanderGugel commented Nov 8, 2016

This has been fixed because we're no longer using distinctKey, which has been removed from RxJS.

Unfortunately Node decided to break symlinks as of this PR: nodejs/node#8749 (comment)

This means package managers that make heavy use of symlinked dependencies won't work in newer versions of Node anymore. This includes PNPM and Yarn.

@alexanderGugel
Copy link
Owner

To follow up, this is actually a bigger issue which would need to be addressed by using a different installation strategy that doesn't rely on symlinks.

@henryruhs
Copy link
Author

henryruhs commented Nov 8, 2016

Let me understand this!? Doesn't this remove all the performance benefit of IED?

I suggest you enable distinctKey and switch to [email protected] for a temporary working IED?

@alexanderGugel
Copy link
Owner

alexanderGugel commented Nov 8, 2016

No, but we need to move away from symlinks.

I'm not happy about this either and I've been trying to prevent this change in Node from the start.

In fact, the issue addressing this is currently one of the most-commented on issues on the Node issue tracker.

The fundamental problem is that Node used to identify required modules by their real path, rather by the path of their symlink (it currently still does).

Eventually the decision was made to use the path of the symlink for some rather obscure reasons. After this broke all kinds of packages (not just package managers like ied and pnpm), the PR was eventually reverted. To allow people to use the "new" behaviour, a new flag was added.

Edit: I just noticed this is completely unrelated to your issue and that I haven't slept for quite some time. So don't get too confused about my above comment.

@zkochan
Copy link
Collaborator

zkochan commented Nov 8, 2016

I think it is fine that it is on a flag for now. With enough time we will be able to prove how awesome is the concept of a shared storage for packages!

I did not see any plans about deprecating --preserve-symlinks. Just this comment

However, even if they plan to do it, they will have to do it as a breaking change, so it'll be not sooner than in version 8. Hence we have like a year to make yarn/pnpm popular enough to be taken into account by the Node.js team.

That's why I'd really like to collaborate with you guys to maybe merge pnpm and yarn or have some shared core or specs... to unite forces so to say

@alexanderGugel
Copy link
Owner

Well, I don't think yarn and ied are going to merge anytime soon. :neckbeard:

That being said, I'm definitely open to the idea of merging pnpm into ied or vice versa. After all, pnpm started out as a fork of ied! So why not go back to the roots? ☀️

@zkochan
Copy link
Collaborator

zkochan commented Nov 8, 2016

haha, I've meant merging pnpm and ied.

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

No branches or pull requests

3 participants