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

Don’t find node_modules up the directory tree when installing #23

Merged
merged 1 commit into from
Jan 18, 2021

Conversation

lydell
Copy link
Member

@lydell lydell commented Jan 18, 2021

elm-tooling install creates links in node_modules/.bin/.

Currently, it looks up the directory tree (starting from the directory containing elm-tooling.json) and errors out if no node_modules is found.

This is “unsound”. Consider this directory structure:

project/
	package.json
	node_modules/
	appA/
		elm.json
		elm-tooling.json
		src/
	appB/
		elm.json
		elm-tooling.json
		src/

What should project/node_modules/.bin/elm link to? appA’s Elm version or appB’s Elm version?

Besides, elm-tooling.json only affects the current project. So elm-tooling install shouldn’t touch stuff outside the project.

This PR fixes the node_modules locator. Now, elm-tooling install does the only correct thing – installs into node_modules in the same directory as elm-tooling.json. If node_modules doesn’t exist it is created.

Note: This has an interesting secondary effect. Yarn 2 does not create a local node_modules folder. So if you install elm-tooling using Yarn 2, you’ll now get a local node_modules folder. It will only contain a .bin/ folder with links to your tools. This should be fine, and even wanted. Editors support node_modules/.bin/ out of the box. In other words, it should now be possible to use elm-tooling with Yarn 2 without getting errors.

@lydell lydell merged commit 6752b89 into main Jan 18, 2021
@lydell lydell deleted the fix-node_modules branch January 18, 2021 17:32
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.

1 participant