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

Extract node-haste to new metro-dependency-graph module #340

Closed
wants to merge 1 commit into from

Conversation

skevy
Copy link
Contributor

@skevy skevy commented Dec 23, 2018

Summary

I've been meaning to do this for a while, and finally got around to finishing this PR.

I'd like to use the Metro dependency graph as a separate package for some work at Airbnb. This PR extracts all the existing dependency graph code (previously in node-haste directory) to a new metro-dependency-graph package.

No functionality should have changed, and besides changing imports, most of this diff should just be shifting files around.

It's worth noting that there may be follow ups needed to make this module truly useful outside of Metro, but this is step 1.

Test plan

Run tests.

cc @mjesun @rafeca

@skevy skevy changed the title Extract node-haste to new 'metro-dependency-graph' module Extract node-haste to new metro-dependency-graph module Dec 23, 2018
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 23, 2018
@codecov-io
Copy link

Codecov Report

Merging #340 into master will decrease coverage by 0.3%.
The diff coverage is 80.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #340      +/-   ##
==========================================
- Coverage   85.06%   84.76%   -0.31%     
==========================================
  Files         172      175       +3     
  Lines        5223     5296      +73     
  Branches      797      800       +3     
==========================================
+ Hits         4443     4489      +46     
- Misses        693      715      +22     
- Partials       87       92       +5
Impacted Files Coverage Δ
...raph/src/DependencyGraph/DependencyGraphHelpers.js 83.33% <ø> (ø)
.../metro-dependency-graph/src/FilesByDirNameIndex.js 18.18% <ø> (ø)
...-dependency-graph/src/lib/parsePlatformFilePath.js 100% <ø> (ø)
...ncy-graph/src/DependencyGraph/ResolutionRequest.js 89.28% <ø> (ø)
...metro-dependency-graph/src/AssetResolutionCache.js 100% <ø> (ø)
packages/metro-dependency-graph/src/ModuleCache.js 75.86% <ø> (ø)
...ages/metro-dependency-graph/src/DependencyGraph.js 83.09% <ø> (ø)
...kages/metro-dependency-graph/src/lib/AssetPaths.js 91.3% <ø> (ø)
.../metro-dependency-graph/src/lib/MapWithDefaults.js 100% <ø> (ø)
...ency-graph/src/DependencyGraph/ModuleResolution.js 86.88% <ø> (ø)
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a49218...987f8a3. Read the comment docs.

@cpojer
Copy link
Contributor

cpojer commented Feb 25, 2019

@mjesun I know you had some plans around this code, could you comment here and then make a decision on this PR?

@skevy
Copy link
Contributor Author

skevy commented Mar 21, 2019

@cpojer @mjesun any updates?

@cpojer
Copy link
Contributor

cpojer commented Mar 21, 2019

I'm happy to finally get rid of the "node-haste" folder. If you rebase, I can land it.

@skevy skevy force-pushed the extra-metro-dependency-graph branch from 987f8a3 to 82f7d77 Compare March 21, 2019 23:59
@cpojer
Copy link
Contributor

cpojer commented Apr 9, 2019

Dang, sorry @skevy I didn't get a notification when you pushed. Do you mind doing one more rebase and then commenting here so I can actually merge it?

@cpojer
Copy link
Contributor

cpojer commented Aug 5, 2019

@skevy are you still interested in landing this?

@cpojer
Copy link
Contributor

cpojer commented Sep 11, 2019

I guess not :(

@cpojer cpojer closed this Sep 11, 2019
@skevy
Copy link
Contributor Author

skevy commented Sep 12, 2019

@cpojer ha sorry I missed your messages. I am actually interested in finishing this, just not sure when I’ll have time to do it. Closing for now is totally fine — I’ll get it done one of these days! Thanks for keeping me honest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants