add optional dependency support #511
Conversation
facebook-github-bot
added
the
CLA Signed
Codecov Report
@@ Coverage Diff @@
## master #511 +/- ##
==========================================
+ Coverage 83.98% 84.04% +0.05%
==========================================
Files 175 175
Lines 5865 5891 +26
Branches 973 979 +6
==========================================
+ Hits 4926 4951 +25
- Misses 827 828 +1
Partials 112 112
Continue to review full report at Codecov.
|
|
It's a very busy time for me at Facebook right now so I don't have enough time to thoroughly review. I left some initial comments. We also cannot make this a default in Metro, at least not in the same diff. Could you make it so the changes do not affect how Metro works in this PR for now? |
I agree, it will be safer this way, the
I understand you have other important things to take care of, and thanks to help moving forward this issue during the busy time... feel free to suggest others to review if that would make it easier (?). |
Codecov Report
@@ Coverage Diff @@
## master #511 +/- ##
==========================================
+ Coverage 83.98% 84.06% +0.07%
==========================================
Files 175 176 +1
Lines 5865 5923 +58
Branches 973 987 +14
==========================================
+ Hits 4926 4979 +53
- Misses 827 832 +5
Partials 112 112
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #511 +/- ##
=======================================
Coverage 84.06% 84.06%
=======================================
Files 176 176
Lines 5923 5923
Branches 987 987
=======================================
Hits 4979 4979
Misses 832 832
Partials 112 112Continue to review full report at Codecov.
|
|
is there a way to install this on a current react-native project? |
not easily... react-native is behind metro version this PR is based on, once this PR is merged we still need to wait for react-native to adopt... |
|
this PR has been sitting for a while, @cpojer is there any concrete concern I can address so we can move this PR forward? |
| TryStatement(path: Path, state: State) { | ||
| path.get('block').traverse({ | ||
| CallExpression(p) { | ||
| const getOptionalDependency = (): string | null => { |
There was a problem hiding this comment.
Can you confirm this only evaluates one level deep in try-blocks and it will not work within nested-if statements or function bodies? I think this makes a lot of sense as it reduces complexity somewhat.
There was a problem hiding this comment.
"one-level deep"? in terms of runtime call-hierarchy or the code-structure?
- code-structure: every statement within the lexical try-block will be parsed (by babel parser), so it is not shallow.
- call-hierarchy: babel parser is a lexical analyzer so it does not evaluate the statements nor following function calls, so you can say it is shallow in terms of call-stack?
example:
function outside() {
import('not-optional");
}
try {
import('this-is-optional');
if(true){
if(false) {
import('this-is-also-optional');
}
}
outside();
} catch (e) {}
The example above is merely for illustration... usually, an optional dependency is enclosed in a much tighter try-block since it needs very specific "catch" logic to handle the missing module situation ...
There was a problem hiding this comment.
Can we instead change it to check only for require calls directly within one-level of a try-block?
Example:
// Valid
let a;
try {
a = require('module');
} catch {}
// Invalid
try {
if ('maybeInclude') {
require('module');
}
} catch {}
You can achieve this by directly iterating over the items in block instead of using traverse.
I think we can reduce complexity here – I don't see any case where a try block for an optional dependency should be more complex than assigning a variable.
There was a problem hiding this comment.
Can we instead change it to check only for require calls directly within one-level of a try-block?... I think we can reduce complexity here
we could certainly constraint to one-level, but what does 'complexity' mean?
try {
// hundreds of line of code here
...
a = require('module');
// another hundred of line of code here
...
} catch {}
The example above is one-level, but is it considered complex? Maybe we should also consider limiting the try-block statement number to be 1 to make it not-complex? But a single statement can also be "complex", so goes the rabbit hole...
On the other hand, if the goal of trying to "reduce complexity" is to make sure developers won't accidentally put the non-optional dependency in the try-block, then instead of using the level, or any other kinds of complexity measurement to guess, I wonder would it be more direct and fool-proof to use an explicit token, such as a comment to safeguard such intention:
try {
// metro: optional-dependency
a = require('this-is-optional');
b = require('this-is-NOT-optional');
} catch {}
So all the existing dependency in try-block, even when the allowOptionalDependencies is turned on, will not be optional, unless developers explicitly added this comment directly above the call... Does that address your concern?
There was a problem hiding this comment.
I am fine if we limit it to variable assignments one-level deep:
try {
a = require('someModule');
b = require('someOtherModule');
} catch {}
Invalid:
try {
a = require('someModule');
runSomeCode();
b = require('someOtherModule');
} catch {}
Any other code can be moved out of the try block and handled afterwards. That way we ensure optional dependencies can only be used via a very single pattern – it's an escape hatch with limited API support.
There was a problem hiding this comment.
ok, I refactor the code to adopt the bottom-up approach and piggyback on the existing import/require expression processing to completely removed the TryStatement handler (top-down). The optional dependency check is now simply to see if the TryStatement is in its "immediate" parent chain after the import/require expression is vetted in the existing CallExpression handlers.
However, I didn't implement any other sibling level constraint from the "invalid" example above. Sibling level constraint will make otherwise simple and linear parent-child traversal a lot more complicated, because it made the expression condition depends on all the other expressions/statements within the block... it didn't add any feature as far as I can see and certainly didn't further reduce the complexity in the extraction code, our ultimate goal...
Let me know if this makes sense and I will do the final rebase/merge.
There was a problem hiding this comment.
This sounds great actually. Can you rebase so I can take another look before importing this to FB?
packages/metro/src/ModuleGraph/worker/__tests__/collectDependencies-test.js
Outdated
Show resolved
Hide resolved
| @@ -349,6 +349,42 @@ it('collects imports', () => { | |||
| ]); | |||
| }); | |||
|
|
|||
| describe('optional dependency', () => { | |||
There was a problem hiding this comment.
| describe('optional dependency', () => { | |
| describe('optional dependencies', () => { |
There was a problem hiding this comment.
Can you expand the test cases here to:
- Verify that nested if statements and function bodies in a try-catch block do not count dependencies as optional.
- Verify that the behavior of this extraction is the same as it is on master now with this option disabled.
There was a problem hiding this comment.
excellent questions...
Verify that nested if statements and function bodies in a try-catch block do not count dependencies as optional.
How do we know the import/require within the if-statement or function bodies are not intended to be optional if they are within the lexical try-block ? Note dependency added by a function declared outside of try-block, even if it is invoked within a try-block, will not be considered optional. If we are worrying about that developers accidentally included an import within the try block that does not mean to be optional, maybe we should just giving warning messages for all the unresolved optional packages?
Verify that the behavior of this extraction is the same as it is on master now with this option disabled.
even though I have verified with tests, there is always the possibility for edge cases we haven't thought about... the only sure way is probably to turn off the whole tryStatement processing when the flag is off, I will do that...
There was a problem hiding this comment.
For the first question: see my answer above about changing the extractor.
| allowOptionalDependencies !== false && | ||
| !( | ||
| Array.isArray(allowOptionalDependencies.exclude) && | ||
| allowOptionalDependencies.exclude.includes(relativePath) |
There was a problem hiding this comment.
Should we instead invert this and require users to explicitly include the optional dependencies they want to use? That way we ensure users will update a configuration every time they use an optional dependency and that they will never end up with a broken bundle that is missing a dependency accidentally.
There was a problem hiding this comment.
maybe I didn't read your comment correctly... but that is exactly "allowOptionalDependencies.exclude" does: to turn optional dependency to be non-optional, if one chooses to.
But it's not required, because many 3rd-party library users might not know what is considered optional within those libraries... for those use cases, metro should just work (with standard syntax analysis - babel parser - taking the hints from code) without requiring extra user config, IMHO.
did I miss your point?
P.S. I can see the code is a bit hard to follow, I will refactor it a bit...
packages/metro/src/DeltaBundler/__tests__/traverseDependencies-test.js
Outdated
Show resolved
Hide resolved
packages/metro/src/DeltaBundler/__tests__/traverseDependencies-test.js
Outdated
Show resolved
Hide resolved
packages/metro/src/DeltaBundler/__tests__/traverseDependencies-test.js
Outdated
Show resolved
Hide resolved
packages/metro/src/DeltaBundler/__tests__/traverseDependencies-test.js
Outdated
Show resolved
Hide resolved
packages/metro/src/DeltaBundler/__tests__/traverseDependencies-test.js
Outdated
Show resolved
Hide resolved
packages/metro/src/DeltaBundler/__tests__/traverseDependencies-test.js
Outdated
Show resolved
Hide resolved
packages/metro/src/DeltaBundler/__tests__/traverseDependencies-test.js
Outdated
Show resolved
Hide resolved
packages/metro/src/DeltaBundler/__tests__/traverseDependencies-test.js
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #511 +/- ##
==========================================
+ Coverage 84.00% 84.07% +0.06%
==========================================
Files 176 176
Lines 5897 5927 +30
Branches 981 989 +8
==========================================
+ Hits 4954 4983 +29
- Misses 831 832 +1
Partials 112 112 Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #511 +/- ##
=========================================
Coverage ? 84.07%
=========================================
Files ? 176
Lines ? 5926
Branches ? 990
=========================================
Hits ? 4982
Misses ? 832
Partials ? 112
Continue to review full report at Codecov.
|
|
hmmm... after merged with the master, ci failed due to node version incompatibility, which is unrelated to this PR...
The master built has been failing at least since the last commit as well: a2738f3 thoughts? |
connectdotz
force-pushed
the
conditional-deps
branch
from
76fa92b
to
1e8e79e
Compare
|
rebase done |
|
sorry, added one more change to consolidate all optional-dependency validation logic into the I think this concluded my change, please let me know if you see anything else... |
There was a problem hiding this comment.
@cpojer has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@drew-gross Is it possible for you to go to your GitHub settings and revoke the OAuth token that is making these Codecov comments? |
Summary: Changes `collectDependencies` so it is responsible for providing a key with each extracted dependency. This key should be *stable* across builds and when dependencies are reordered, and *must* be locally unique within a given module. NOTE: This is a similar concept to the [`key`](https://reactjs.org/docs/lists-and-keys.html#keys) prop in React. It's also used for a similar purpose - `traverseDependencies` is not unlike a tree diffing algorithm. Previously, the `name` property ( = the first argument to `require` etc) *implicitly* served as this key in both `collectDependencies` and DeltaBundler, but since the addition of `require.context` dependency descriptors in facebook#821, this is no longer strictly correct. (This diff therefore unblocks implementing `require.context` in DeltaBundler.) Instead of teaching DeltaBundler to internally re-derive suitable keys from the dependency descriptors - essentially duplicating the logic in `collectDependencies` - the approach taken here is to require `collectDependencies` to return an *explicit* key as part of each descriptor. NOTE: Keys should be considered completely opaque. As an implementation detail (that may change without notice), we Base64-encode the keys to obfuscate them and deter downstream code from depending on the information in them. (We do this on the assumption that Base64 encoding performs better than hashing.) Note that it's safe for multiple descriptors to resolve to a single module (as of D37194640 (facebook@fc29a11)), so from DeltaBundler's perspective it's now perfectly valid for `collectDependencies` to not collapse dependencies at all (e.g. even generate one descriptor per `require` call site) as long as it provides a unique key with each one. WARNING: This diff exposes a **preexisting** bug affecting optional dependencies (introduced in facebook#511) - see FIXME comment in `graphOperations.js` for the details. This will require more followup in a separate diff. Differential Revision: D37205825 fbshipit-source-id: 3559ee6a2f3c30017812ff009f0fe4c442e30029
Summary: Pull Request resolved: #835 Changes `collectDependencies` so it is responsible for providing a key with each extracted dependency. This key should be *stable* across builds and when dependencies are reordered, and *must* be locally unique within a given module. NOTE: This is a similar concept to the [`key`](https://reactjs.org/docs/lists-and-keys.html#keys) prop in React. It's also used for a similar purpose - `traverseDependencies` is not unlike a tree diffing algorithm. Previously, the `name` property ( = the first argument to `require` etc) *implicitly* served as this key in both `collectDependencies` and DeltaBundler, but since the addition of `require.context` dependency descriptors in #821, this is no longer strictly correct. (This diff therefore unblocks implementing `require.context` in DeltaBundler.) Instead of teaching DeltaBundler to internally re-derive suitable keys from the dependency descriptors - essentially duplicating the logic in `collectDependencies` - the approach taken here is to require `collectDependencies` to return an *explicit* key as part of each descriptor. NOTE: Keys should be considered completely opaque. As an implementation detail (that may change without notice), we hash the keys to obfuscate them and deter downstream code from depending on the information in them. Note that it's safe for multiple descriptors to resolve to a single module (as of D37194640 (fc29a11)), so from DeltaBundler's perspective it's now perfectly valid for `collectDependencies` to not collapse dependencies at all (e.g. even generate one descriptor per `require` call site) as long as it provides a unique key with each one. WARNING: This diff exposes a **preexisting** bug affecting optional dependencies (introduced in #511) - see FIXME comment in `graphOperations.js` for the details. This will require more followup in a separate diff. Changelog: [Internal] Reviewed By: jacdebug Differential Revision: D37205825 fbshipit-source-id: 92dc306803e647b25bd576dae02960215fc01da6
Summary
This PR added support for the common optional dependency pattern: enclose require() within try/catch block, which is already supported by webpack.
While the lack of optional dependency support is not new, due to lean-core initiative, it has become obvious that we should just address this to provide better customer-experience. See the excellent discussion in react-native-community/discussions-and-proposals#120.
Change Outline
The changes are mainly in 3 areas:
collectDependencies.js)traverseDependencies.js)optionalDependency, defaults to true) to disable and customize (exclude dependency from being optional) (metro-config)The rest are just tunneling through the new config option, added/modified tests.
Test plan
tested the new create react-native app with
react-native-vector-iconsand optional@react-native-community/toolbar-android(in try/catch block fromreact-native-vector-icons) :@react-native-community/toolbar-androidnot found and abortDiscussion
import()in the try block, should we also consider it "optional"? The common pattern is to userequire()but one would thinkimport()should be just the same, no? This PR considered any dependency found in the try block "optional", but that can be changed easily.optionalDependencyin the config.resolver, not sure if it is best to be in resolver or transformer?