search

rrdelaney / reason-scripts

:beginner: Create a ReasonML and React development environment
MIT License
628 stars 58 forks source link

Avoid npm link bs-platform if possible #20

Closed zploskey closed 6 years ago

zploskey commented 6 years ago

Is it possible to avoid doing npm link bs-platform? When bs-platform is already globally installed using yarn global add bs-platform, npm link will overwrite the symlinks that used to point to the globally installed yarn version's binaries. This is probably not what the user wanted to happen. Ideally, bs-platform would would not care which package manager installed it and would not call a package manager directly.

This is something that bs-platform itself does, but we do it here as well in the prepare step. See https://github.com/BuckleScript/bucklescript/issues/1972.

rvcas commented 6 years ago

@zploskey I can support that. I just went through some tedious stuff till I figured out that "prepare": "npm link bs-platform" was causing me problems. I have bs-platform installed globally through yarn. Website for reason says to use the repo url when installing bs-platform. I think npm link was auto-installing bs-platform from npm (apparently buggy) because I did not have it globally installed through npm -g. @rrdelaney Is there any reason not to pull bs-platform as a dev dependency instead of using it globally?

rrdelaney commented 6 years ago

The reason we're using npm link bs-platform instead of adding it as a dev dependency is bs-platform has a really long postinstall where it compiles the compiler. This isn't a huge deal for npm users, but Yarn will re-run that postinstall everytime you add a new dependency for that project. It has some problems, but right now the best bet is to just link globally.

Here's the Yarn issue for reference: https://github.com/yarnpkg/yarn/issues/932

zploskey commented 6 years ago

Thanks for explaining why it was set up this way. Sounds like that yarn bug is a blocker for this. The way the discussion has been going in there, it looks like the problem and potential solution have been identified, so hopefully a fix for it gets committed soon.

rrdelaney commented 6 years ago

Yep! As soon as that Yarn bug is ironed out we should be able to just use it as a dev dependency, or if bs-platform ships precompiled binaries

rvcas commented 6 years ago

cool thanks. maybe close this @zploskey ?

zploskey commented 6 years ago

This is the kind of thing I would normally leave open as a reminder to fix once it gets unblocked. I'll leave the decision to others if they prefer to close it.

zploskey commented 6 years ago

This was fixed in 2207dfcbd4a2d664bd47d68389ddee9db85d5b84. Thanks @rrdelaney.