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

chore: add ink and react peer dependencies #5

Merged
merged 1 commit into from
Apr 13, 2019

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Apr 12, 2019

In some cases right now packages might be installed weirdly in node_modules - especially if you end up in 2 separate instances of react (1 for frontend code and 1 for terminal utility using ink). This seems to give hint to package managers (tested with yarn and npm on how to structure packages in node_modules).

Some background on this - in https://github.com/gatsbyjs/gatsby we want to move our CLI to use ink, but because primarly gatsby is tool for frontend where user specify react version (it's not bundled with gatsby). We might end up with 2 react instances if user is using react version lower than 16.8.0 (which is specified is minimal peer dependency in ink ( https://github.com/vadimdemedes/ink/blob/master/package.json#L84-L86 ). Because of that ink component packages might end up being hoisted to top level node_modules but ink and react itself might be in node_modules/gatsby-cli/node_modules, which cause "Module not found" errors from ink-spinner when it tries to import from those modules.

I was testing this before with forked/scoped of ink-spinner ( https://unpkg.com/@pieh/[email protected]/package.json ) and it seems to solve issues we saw, when react@<16.8.0 was used.

I might not sure on exact versions of ink and react to chose for peer dependencies - so I choose lowest ink and same react as in devDeps.

@pieh
Copy link
Contributor Author

pieh commented Apr 12, 2019

Is the failed test a flake? It didn't happen locally, and also my changes to add peerDependencies shouldn't affect those I think?

@vadimdemedes
Copy link
Owner

Thanks for this PR! It's weird that tests started failing, I guess one of my recent changes to Ink could've caused it. Going to merge this anyway and fix tests afterwards.

@vadimdemedes vadimdemedes merged commit dec78f5 into vadimdemedes:master Apr 13, 2019
@vadimdemedes
Copy link
Owner

Couldn't fix the tests on CI yet (they pass locally), so I did a release anyway - 3.0.1.

@Mark1626 Mark1626 mentioned this pull request Nov 24, 2019
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