Skip to content

Conversation

@moberegger
Copy link
Contributor

@moberegger moberegger commented Feb 24, 2019

Removes the dependency on tmeasday:check-npm-versions. This will save ~10kb in the bundle payload and save a small runtime cost.

An argument can be made that check-npm-modules doesn't provide much value when compared to what the Meteor build tool already does. Without this, Meteor will already display an error message

Cannot find module 'react'

In my opinion, that is clear enough and the error provided by check-npm-versions doesn't provide much more insight - at least not enough to warrant the weight it adds to the production build. Furthermore, if you are using this package, it is very likely that you already have react installed; I can't imagine a developer getting very far without having react installed... at least not far enough to use withTracker in any capacity.

Whilst it is nice to get this error at build time during development, I don't think the tradeoff is worth it, especially considering that it would only be helpful in very rare circumstances.

@apollo-cla
Copy link

@moberegger: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@moberegger moberegger changed the title Remove react-mateor-data's dependency on check-npm-dependencies Remove react-meteor-data's dependency on check-npm-dependencies Feb 24, 2019
Copy link
Contributor

@benjamn benjamn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I agree with your assessment!

@yched
Copy link
Contributor

yched commented Mar 31, 2019

#262 will require React 16.8+ (the PR currently bumps the version in the current checkNpmVersions() call)

Do we have other ways to express that if we remove check-npm-modules ?

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

1 similar comment
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@filipenevola
Copy link
Contributor

closing because of [email protected] #273

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.

6 participants