Remove fbjs dependency #13069
Remove fbjs dependency #13069
Conversation
|
React: size: 🔺+0.1%, gzip: 0.0% ReactDOM: size: 0.0%, gzip: -0.1% Details of bundled changes.Comparing: b1b3acb...5ebb27b react
react-dom
react-art
react-test-renderer
react-reconciler
simple-cache-provider
create-subscription
react-native-renderer
react-scheduler
Generated by 🚫 dangerJS |
|
Btw I've no idea what's causing the size changes. Some CommonJS interop junk? Would be interesting to dive in. |
|
There are already https://www.npmjs.com/package/warning |
|
We want to go in the other direction. These are just internal utilities and there’s nothing special about them that we’d want other people to rely on. |
|
The size is reduced because babel helpers are reused across whole bundle. In fbjs they look like inlined. |
|
We’re talking about ~50 bytes here. This doesn’t make a practical difference. |
|
It's better to tell them because they do not accept any improvements like esm support. |
|
I don't understand what it is that you'd like us to on our side. We don't recommend anyone to use or depend on these modules. They're internal to React. If somebody decides to publish a copy it's their business—you're not obligated to use them either :-). We want to stop depending on fbjs because fbjs includes a bunch of dependencies that we don't use, and so every install of React has to download them in vain (this doesn't have to do with bundle size, just package management). It's annoying when some transitive deps are flagged as having security issues, but again, we don't actually use them. So that's part of motivation for us to get rid of fbjs dependency. Another part is that we might want to make more changes to our invariant/warning setup, potentially with React-specific aspects. This is harder to do when they're centralized like this and used by everyone. So we want to have more freedom by making them private. |
|
I think it'd be worth broadcasting this change to the community. There are likely a number of packages that import from |
|
Umm.. That was always a bad idea. I'm worried that "announcing" this might make it look like we're intentionally breaking something, and trying to "smooth it over". Whereas in practice anybody who relies on this has a misunderstanding of how npm works. |
|
I know it's a bad idea, but I've seen it before 🙂 Maybe it's uncommon enough to not worry about it. |
|
I think it's reasonable: From the readme
|
|
IMHO it'd be great to make invariant warning official react packages, either taking over those npm repos or calling them Not only are they quite popular in the React ecosystem and it'd be great if there was more code sharing going on here, they're also used by other React projects from Facebook like prop-types and React Native. |
|
@realityking This already was discussed above |
|
@TrySound I saw that but it doesn't address the usage in other React projects like Native and prop-types. |
|
There's just no reason for anyone to want to use our Inside React, they have a specific purpose. We use React Native might also inline them later. PropTypes can be changed to use |
There was a problem hiding this comment.
I dig it.
I think we can now remove the "fbjs" dependency in our release script?
| @@ -237,6 +237,20 @@ function isProfilingBundleType(bundleType) { | |||
| } | |||
| } | |||
|
|
|||
| function blacklistFBJS() { | |||
| 'prop-types/checkPropTypes': HAS_NO_SIDE_EFFECTS_ON_IMPORT, | ||
| 'fbjs/lib/camelizeStyleName': HAS_NO_SIDE_EFFECTS_ON_IMPORT, | ||
| 'fbjs/lib/hyphenateStyleName': HAS_NO_SIDE_EFFECTS_ON_IMPORT, | ||
| deepFreezeAndThrowOnMutationInDev: HAS_NO_SIDE_EFFECTS_ON_IMPORT, |
|
PropTypes PR: facebook/prop-types#194 |
* Inline fbjs/lib/invariant * Inline fbjs/lib/warning * Remove remaining usage of fbjs in packages/*.js * Fix lint * Remove fbjs from dependencies * Protect against accidental fbjs imports * Fix broken test mocks * Allow transitive deps on fbjs/ for UMD bundles * Remove fbjs from release script
This removes our runtime dependency on fbjs for a few reasons:
I had to fork www's invariant and warning. Technically they were already implicitly forked (because www's CommonJS resolution gave us a different module) so this is just making it implicit. Warning needs to be forked because of blacklists, and invariant needs to be forked because of differences in production argument formatting logic (which goes through
exinternally).Note: to fully get rid of it we'd also need to remove it in
prop-types(facebook/prop-types#194).