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

Parse destructuring of assignment statements. #5483

Merged

Conversation

seanparsons
Copy link
Contributor

@seanparsons seanparsons commented May 2, 2024

Problem:
We need more detail in the assignment statements so that we can track destructuring.

Fix:
We already have Param and some associated types which handle the destructuring for function parameters and the TypeScript compiler uses the same underlying representation in both cases (function parameters and variable declarations). As a result in this we use that existing functionality to handle the same type of parsing for the variable declarations.

Commit Details:

  • Changed JSAssignment.leftHandSide to be an instance of BoundParam.
  • Widened the type of some Param related fields from JSExpressionMapOrOtherJavascript to JSExpression.
  • Added clearArbitraryJSBlockSourceMaps, clearDestructuredArrayPartSourceMaps, clearDestructuredParamPartSourceMaps, clearParamSourceMaps, clearBoundParamSourceMaps,
    clearBoundParamUniqueIDsAndSourceMaps, clearTopLevelElementSourceMaps, clearJSAssignmentSourceMaps, clearJSArbitraryStatementSourceMaps, clearParseResultSourceMapsUniqueIDsAndEmptyBlocks and clearTopLevelElementSourceMapsUniqueIDsAndEmptyBlocks utility functions.
  • parseDeclaration now uses parseBindingName to produce the expected representation for JSAssignment values.
  • Integrated the parsing into lookupIntoComponentScope.

Manual Tests:
I hereby swear that:

  • I opened a hydrogen project and it loaded
  • I could navigate to various routes in Preview mode

Copy link
Contributor

github-actions bot commented May 2, 2024

Try me

Copy link

relativeci bot commented May 2, 2024

#12371 Bundle Size — 62.48MiB (+0.03%).

5c8e5d0(current) vs 6339ece master#12344(baseline)

Warning

Bundle contains 58 duplicate packages – View duplicate packages

Bundle metrics  Change 3 changes Regression 1 regression
                 Current
#12371
     Baseline
#12344
Regression  Initial JS 45.54MiB(+0.01%) 45.54MiB
No change  Initial CSS 0B 0B
Change  Cache Invalidation 22.23% 21.69%
No change  Chunks 31 31
No change  Assets 34 34
No change  Modules 4374 4374
No change  Duplicate Modules 504 504
Change  Duplicate Code 30.83%(+0.03%) 30.82%
No change  Packages 468 468
No change  Duplicate Packages 58 58
Bundle size by type  Change 2 changes Regression 1 regression Improvement 1 improvement
                 Current
#12371
     Baseline
#12344
Regression  JS 62.47MiB (+0.03%) 62.45MiB
Improvement  HTML 10.94KiB (-0.34%) 10.97KiB

Bundle analysis reportBranch feature/parse-destructuring-of-a...Project dashboard

Copy link
Contributor

github-actions bot commented May 2, 2024

Performance test results:
(Chart1)
(Chart2)

@balazsbajorics balazsbajorics mentioned this pull request May 4, 2024
@seanparsons seanparsons marked this pull request as ready for review May 9, 2024 14:51
Copy link
Contributor

@bkrmendy bkrmendy left a comment

Choose a reason for hiding this comment

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

the code looks good, I'd add a test to the data tracing test suite that uses the new functionality (starting from here

// let's try to find "const { <identifier>, ...somethingElse } = <expression>" shaped statements
) with a test case that the old code couldn't have handled

- Prevent tracing through spread objects or arrays.
- Widen `hookCallRegex` a little.
@seanparsons seanparsons merged commit 0b52dbc into master May 13, 2024
18 checks passed
@seanparsons seanparsons deleted the feature/parse-destructuring-of-assignment-statements branch May 13, 2024 11:28
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.

Data Tracing: be able to trace const { myData } = useLoaderData(); and others
4 participants