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

shouldVisitChild: Check parent and child node previously deferred separately #7043

Merged
merged 6 commits into from
Oct 12, 2021

Conversation

wbinnssmith
Copy link
Contributor

@wbinnssmith wbinnssmith commented Oct 8, 2021

Follows up and fixes an issue in #7035 that would result in "Asset was skipped or not found." by unmarking asset group parents when the asset group was deferred, not the dependency.

Test Plan:

  • Test in large app
  • Integration test

@height
Copy link

height bot commented Oct 8, 2021

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

@wbinnssmith wbinnssmith marked this pull request as draft October 8, 2021 19:49
@parcel-benchmark
Copy link

parcel-benchmark commented Oct 8, 2021

Benchmark Results

Kitchen Sink ✅

Timings

Description Time Difference
Cold 1.74s -40.00ms
Cached 288.00ms -17.00ms 🚀

Cold Bundles

No bundle changes detected.

Cached Bundles

Bundle Size Difference Time Difference
dist/legacy/index.96016b08.js 1.59kb +0.00b 771.00ms -60.00ms 🚀
dist/legacy/index.8aaa89c9.js 1.20kb +0.00b 771.00ms -59.00ms 🚀
dist/modern/index.6be20f01.js 1.13kb +0.00b 770.00ms -60.00ms 🚀
dist/legacy/index.html 826.00b +0.00b 890.00ms -62.00ms 🚀
dist/modern/index.html 749.00b +0.00b 889.00ms -63.00ms 🚀
dist/legacy/index.c1bc86aa.css 94.00b +0.00b 1000.00ms -76.00ms 🚀
dist/modern/index.57a95cbe.css 94.00b +0.00b 999.00ms -76.00ms 🚀

React HackerNews ✅

Timings

Description Time Difference
Cold 9.22s -464.00ms 🚀
Cached 519.00ms +70.00ms ⚠️

Cold Bundles

No bundle changes detected.

Cached Bundles

Bundle Size Difference Time Difference
dist/index.js 487.00kb +0.00b 4.87s -287.00ms 🚀
dist/PermalinkedComment.62775cc6.js 4.20kb +0.00b 4.87s -287.00ms 🚀
dist/UserProfile.1b7befb5.js 1.57kb +0.00b 4.87s -287.00ms 🚀
dist/NotFound.e6c89571.js 429.00b +0.00b 4.87s -286.00ms 🚀
dist/logo.c5bb83f1.png 246.00b +0.00b 4.82s -292.00ms 🚀

AtlasKit Editor ✅

Timings

Description Time Difference
Cold 1.03m +1.03m ⚠️
Cached 1.66s +1.66s ⚠️

Cold Bundles

No bundles found, this is probably a failed build...

Cached Bundles

No bundles found, this is probably a failed build...

Three.js ✅

Timings

Description Time Difference
Cold 6.36s -61.00ms
Cached 373.00ms -10.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

Click here to view a detailed benchmark overview.

@mischnic mischnic added this to the v2.0.0 milestone Oct 10, 2021
@gorakong gorakong force-pushed the asset-group-deferred-change branch 4 times, most recently from c2af975 to fefa340 Compare October 12, 2021 02:10
@mischnic mischnic force-pushed the asset-group-deferred-change branch 2 times, most recently from 8eee098 to b1d6c6c Compare October 12, 2021 19:49
@mischnic mischnic marked this pull request as ready for review October 12, 2021 19:50
@mischnic
Copy link
Member

Asset graph of the test case. Notice that dependency at the bottom left is marked "hasDeferred" (so deferred) but the symbol "bar" is actually used. This caused the "Asset was skipped or not found" error

AssetGraph_Main

@devongovett devongovett merged commit c78601b into v2 Oct 12, 2021
@devongovett devongovett deleted the asset-group-deferred-change branch October 12, 2021 22:11
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.

None yet

5 participants