View Parser Plugin Decomposition #413

mrigankmg · 2024-07-05T20:26:46Z

This PR refactors the view parser so that parseLocalObject does not contain code to parse specific nodes, but rather delegates that functionality to plugins.

resolves #373
resolves #412

Change Type (required)

Indicate the type of change your pull request is:

patch
minor
major

Does your PR have any documentation updates?

Updated docs
No Update needed
Unable to update docs

Version

Published prerelease version: 0.8.0-next.5

Changelog

Release Notes

Update to use TypeScript 5.5 and enable `isolatedDeclarations` (#431)

Update to use TypeScript 5.5 and enable isolatedDeclarations

bump js rules, use node 20 (#430)

Use Node 20 for builds

JS Package Cleanup (#442)

Fix migration issues in JS packages

Does your PR have any documentation updates?

Updated docs
No Update needed
Unable to update docs

Bazel 6 Migration (#252)

Swaps the repo internals to use bazel@6, rules_js, bazel modules, vitest and tsup for the core + plugin builds

🚀 Enhancement

Bazel 6 Migration #252 (@adierkens @hborawski @brocollie08 @KetanReddy @sugarmanz nancy_wu1@intuit.com @nancywu1 @mercillo @mrigankmg @sakuntala-motukuri)

🐛 Bug Fix

View Parser Plugin Decomposition #413 (@mrigankmg)
Ability to remove a resolved async node #404 (@sakuntala-motukuri @brocollie08)
ReactAsset improve error messages #444 (@cehan-Chloe)
JS Package Cleanup #442 (@KetanReddy)
Specific Temurin Version #445 (@brocollie08)
iOS: Canary release support + SPM automated releases #434 (@hborawski)
apply strategy from pr builds to main & release builds #440 (@sugarmanz)
Add dsl component to async node #439 (@sakuntala-motukuri)
Bazel clean up #435 (@brocollie08)
[JVM] Publish next releases to release repository (fixed versions) #428 (@sugarmanz)
This PR enables to add footer component to info reference asset in android #411 (@sakuntala-motukuri)
Fix NPM Registry URL #427 (@KetanReddy)

🏠 Internal

Use smaller docker image for circle setup #456 (@adierkens)
Log debug info for npm next release #454 (@adierkens)
Remove alternate binding parsers from bundle #432 (@adierkens)
Revert "[JVM] Publish next releases to release repository" #429 (@sugarmanz)
[JVM] Publish next releases to release repository #426 (@sugarmanz)

📝 Documentation

Update CONTRIBUTING.md #433 (@brocollie08)

🔩 Dependency Updates

Update to use TypeScript 5.5 and enable isolatedDeclarations #431 (@KetanReddy)
bump js rules, use node 20 #430 (@KetanReddy @hborawski @mercillo)

Authors: 11

@brocollie08
@nancywu1
Adam Dierkens (@adierkens)
Chloe (@cehan-Chloe)
Harris Borawski (@hborawski)
Jeremiah Zucker (@sugarmanz)
Ketan Reddy (@KetanReddy)
Marlon "Marky" Ercillo (@mercillo)
Mrigank Mehta (@mrigankmg)
nancywu1 (nancy_wu1@intuit.com)
Niharika Motukuri (@sakuntala-motukuri)

codecov · 2024-07-23T20:53:42Z

Codecov Report

Attention: Patch coverage is 96.33508% with 14 lines in your changes missing coverage. Please review.

Project coverage is 91.93%. Comparing base (81061cf) to head (6b54598).

Files	Patch %	Lines
core/player/src/view/plugins/applicability.ts	92.30%	3 Missing ⚠️
core/player/src/view/plugins/asset.ts	92.85%	3 Missing ⚠️
core/player/src/view/plugins/multi-node.ts	97.26%	2 Missing ⚠️
core/player/src/view/plugins/switch.ts	97.53%	1 Missing and 1 partial ⚠️
core/player/src/view/plugins/template.ts	94.73%	2 Missing ⚠️
plugins/async-node/core/src/index.ts	93.54%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #413      +/-   ##
==========================================
+ Coverage   91.84%   91.93%   +0.08%     
==========================================
  Files         338      340       +2     
  Lines       26831    26826       -5     
  Branches     1941     1942       +1     
==========================================
+ Hits        24644    24663      +19     
+ Misses       2173     2149      -24     
  Partials       14       14

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

brocollie08 · 2024-07-25T11:45:55Z

core/player/src/view/__tests__/view.immutable.test.ts

@@ -34,6 +39,7 @@ test("uses the exact same object if nothing changes", () => {
  );

  new StringResolverPlugin().apply(view);
+  new AssetPlugin().apply(view);


a lot of these tests have these new plugins added in, it's because these (Multinode + Asset) used to come for free with the parser, right?
Is there appetite in consolidating them to some test util to some default plugin setup like DefaultViewPlugin to avoid having to remember which ones you need to apply in every test

Do we need to create a separate test util plugin? Could we not use the DefaultViewPlugin itself?

ya that's possibly fine, it's probably registering more plugins than you need for a lot of tests that are very targeted.
Maybe @KetanReddy can chime in here

core/player/src/view/parser/utils.ts

brocollie08 · 2024-07-25T11:55:18Z

core/player/src/view/plugins/__tests__/applicability.test.ts

@@ -245,21 +248,26 @@ describe("applicability", () => {
    });
  });

-  it("determines if nodeType is applicability", () => {


i know we removed the determineNodeType hook, but should we still do some similar type checking by indirectly testing private isApplicability(obj: any)?

How would we test a private function?

you'd call the hook that calls the private function, then verify the side effects of the private function

as discussed in slack, these tests might not be relevant anymore since the result being returned -> isApplicability returned true, as long as we have a test for negative case as well, this is fine

core/player/src/view/plugins/switch.ts

plugins/async-node/core/src/index.test.ts

core/player/src/view/parser/utils.ts

hborawski

🚀 This is a huge refactor, glad to see it fitting properly into the plugin architecture!

mrigankmg changed the title ~~Refactored a bit~~ View arser-plugin-decomposition Jul 8, 2024

mrigankmg changed the title ~~View arser-plugin-decomposition~~ View Parser Plugin Decomposition Jul 8, 2024

mrigankmg force-pushed the view-parser-plugin-decomposition branch 11 times, most recently from 08cb60a to b5b47ae Compare July 17, 2024 16:33

mrigankmg changed the base branch from bazel-6 to main July 17, 2024 17:19

mrigankmg changed the base branch from main to bazel-6 July 17, 2024 17:23

mrigankmg changed the base branch from bazel-6 to main July 17, 2024 17:34

mrigankmg force-pushed the view-parser-plugin-decomposition branch 4 times, most recently from 67ed64a to b806e58 Compare July 19, 2024 20:04

mrigankmg marked this pull request as ready for review July 23, 2024 13:50

mrigankmg requested review from KetanReddy and adierkens as code owners July 23, 2024 13:50

mrigankmg requested a review from hborawski July 23, 2024 13:50

mrigankmg force-pushed the view-parser-plugin-decomposition branch 6 times, most recently from 29767b0 to 0aad1eb Compare July 23, 2024 19:32

mrigankmg force-pushed the view-parser-plugin-decomposition branch from 0aad1eb to 19bb017 Compare July 23, 2024 20:10

mrigankmg force-pushed the view-parser-plugin-decomposition branch 4 times, most recently from 8940520 to 3183681 Compare July 24, 2024 14:36

hborawski requested a review from brocollie08 July 24, 2024 16:28

mrigankmg force-pushed the view-parser-plugin-decomposition branch 2 times, most recently from dd3b0fc to 60535ed Compare July 24, 2024 16:46

Refactored view parser into plugin decomposition

0ed9b29

mrigankmg force-pushed the view-parser-plugin-decomposition branch 2 times, most recently from 4c37617 to 96863cd Compare July 24, 2024 21:11

brocollie08 reviewed Jul 25, 2024

View reviewed changes

mrigankmg force-pushed the view-parser-plugin-decomposition branch 2 times, most recently from e8b8f0d to 0508ecf Compare July 25, 2024 17:58

KetanReddy reviewed Jul 25, 2024

View reviewed changes

core/player/src/view/parser/utils.ts Show resolved Hide resolved

Merge branch 'main' into view-parser-plugin-decomposition

6b54598

mrigankmg force-pushed the view-parser-plugin-decomposition branch from 0508ecf to 6b54598 Compare July 26, 2024 14:10

hborawski approved these changes Jul 26, 2024

View reviewed changes

brocollie08 approved these changes Jul 29, 2024

View reviewed changes

mrigankmg merged commit b2d782d into main Jul 29, 2024
11 checks passed

mrigankmg deleted the view-parser-plugin-decomposition branch July 29, 2024 17:19

This was referenced Jul 30, 2024

Fixed switch parsing syntax issue for dynamic switches #458

Merged

Fix J2V8 Debugger #443

Merged

Update rules1.1.2 #473

Merged

Docs and Release Fixes #474

Merged

Docsite Fixes #475

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

View Parser Plugin Decomposition #413

View Parser Plugin Decomposition #413

mrigankmg commented Jul 5, 2024 •

edited by intuit-svc

Loading

codecov bot commented Jul 23, 2024 •

edited

Loading

brocollie08 Jul 25, 2024

mrigankmg Jul 25, 2024 •

edited

Loading

brocollie08 Jul 25, 2024

brocollie08 Jul 25, 2024

mrigankmg Jul 25, 2024 •

edited

Loading

brocollie08 Jul 25, 2024

brocollie08 Jul 25, 2024

hborawski left a comment

View Parser Plugin Decomposition #413

View Parser Plugin Decomposition #413

Conversation

mrigankmg commented Jul 5, 2024 • edited by intuit-svc Loading

Change Type (required)

Does your PR have any documentation updates?

Version

Release Notes

Update to use TypeScript 5.5 and enable isolatedDeclarations (#431)

bump js rules, use node 20 (#430)

JS Package Cleanup (#442)

Does your PR have any documentation updates?

Bazel 6 Migration (#252)

🚀 Enhancement

🐛 Bug Fix

🏠 Internal

📝 Documentation

🔩 Dependency Updates

Authors: 11

codecov bot commented Jul 23, 2024 • edited Loading

Codecov Report

brocollie08 Jul 25, 2024

Choose a reason for hiding this comment

mrigankmg Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

brocollie08 Jul 25, 2024

Choose a reason for hiding this comment

brocollie08 Jul 25, 2024

Choose a reason for hiding this comment

mrigankmg Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

brocollie08 Jul 25, 2024

Choose a reason for hiding this comment

brocollie08 Jul 25, 2024

Choose a reason for hiding this comment

hborawski left a comment

Choose a reason for hiding this comment

mrigankmg commented Jul 5, 2024 •

edited by intuit-svc

Loading

Update to use TypeScript 5.5 and enable `isolatedDeclarations` (#431)

codecov bot commented Jul 23, 2024 •

edited

Loading

mrigankmg Jul 25, 2024 •

edited

Loading

mrigankmg Jul 25, 2024 •

edited

Loading