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

Move react-devtools-core to optional peer dependency #498

Merged

Conversation

SimenB
Copy link
Contributor

@SimenB SimenB commented Feb 10, 2022

Fixes #491.

Should possibly wrap

ink/src/reconciler.ts

Lines 24 to 30 in 763db80

// We need to conditionally perform devtools connection to avoid
// accidentally breaking other third-party code.
// See https://github.com/vadimdemedes/ink/issues/384
if (process.env.DEV === 'true') {
// eslint-disable-next-line import/no-unassigned-import
require('./devtools');
}
into a more specific error as well?

@vadimdemedes
Copy link
Owner

Should possibly wrap into a more specific error as well?

You mean something like this?

try {
	require('react-devtools-core');
} catch {
	throw new Error('React Devtools requires `react-devtools-core` dependency to be installed.');
}

@SimenB
Copy link
Contributor Author

SimenB commented Feb 11, 2022

Yeah, makes sense to me. Or possibly just a console.warn? Unless process.env.DEV === 'true' is solely used for devtools and have no other purpose, then throwing a descriptive error seems fine 🙂

@vadimdemedes
Copy link
Owner

Yep, DEV environment variable is only used for devtools. However, I like the idea of just having console.warn, because technically nothing is broken, so there's no reason to crash the whole app.

Let's go with console.warn 👍 Could you extend the error message to:

Debugging with React Devtools requires `react-devtools-core` dependency to be installed.

$ npm install --save-dev react-devtools-core

@vadimdemedes vadimdemedes changed the title fix: move react-devtools-core to optional peer dependency Move react-devtools-core to optional peer dependency Feb 11, 2022
@SimenB
Copy link
Contributor Author

SimenB commented Feb 11, 2022

image

Seems fine?

@vadimdemedes
Copy link
Owner

Yes, thank you!

@vadimdemedes vadimdemedes merged commit 8c2378a into vadimdemedes:master Feb 17, 2022
@SimenB SimenB deleted the move-react-devtools-to-peer-dep branch February 17, 2022 13:46
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.

react-devtools-core adds 14 MB to every installation using Ink
2 participants