Navigation Menu

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

Remove fbjs dependency #194

Merged
merged 2 commits into from Jun 19, 2018
Merged

Remove fbjs dependency #194

merged 2 commits into from Jun 19, 2018

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented Jun 19, 2018

See reasons in facebook/react#13069.
This PR is complementary.

I chose to inline a simplified version of warning in two files where we use it, and replacing invariant calls with new Error. In case somebody relies on Invariant Violation being the name (e.g. in tests), I inlined .name assignments to match the invariant logic.

Without this PR, fbjs would stay a transitive dep of React.

// --- Welcome to debugging React ---
// This error was thrown as a convenience so that you can use this stack
// to find the callsite that caused this warning to fire.
throw new Error(message);
Copy link
Collaborator

Choose a reason for hiding this comment

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

the intention here is for the debugger to pause on it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. (Not new code, just copy paste from fbjs/lib/warning)

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

LGTM

'function must return `null` or an `Error` but returned a ' + typeof error + '. ' +
'You may have forgotten to pass an argument to the type checker ' +
'creator (arrayOf, instanceOf, objectOf, oneOf, oneOfType, and ' +
'shape all require an argument).'
Copy link
Contributor

@bvaughn bvaughn Jun 19, 2018

Choose a reason for hiding this comment

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

😅 Reading this kind of makes me wish you'd also copied over the format-args functionality too heh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did at first, but Babel output was super ugly and I wasn't confident enough to write the same by hand (there was both some spreading and some resting) and not break something. And we can't use ES6 because we haven't set up a pipeline here 😛

var printWarning = function() {};

if (process.env.NODE_ENV !== 'production') {
printWarning = function(text) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why copy this in each file? No big deal, since it's small.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about putting into prop-types/warning but then figured someone will start using that as a "public" API. Might as well just tuck it away.

@MariaDima
Copy link

Can someone please make a release, so that all modules that depend on prop-types can get the vulnerability fix?

@gaearon
Copy link
Contributor Author

gaearon commented Jun 28, 2018

  1. This PR was released as 15.6.2.
  2. There's no (and has never been) a vulnerability in prop-types. Even if some transitive dependency of fbjs had a vulnerability at some point, prop-types only uses functions in fbjs that have no transitive dependencies.

@gaearon gaearon deleted the no-fbjs branch June 28, 2018 16:10
@ljharb
Copy link
Collaborator

ljharb commented Jun 28, 2018

@gaearon could you push all the git tags up for the releases? It seems that most of them (including the latest) are missing.

@gaearon
Copy link
Contributor Author

gaearon commented Jun 28, 2018

Pushed a tag for the latest release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants