Skip to content

Fix Parser instantiation and globals in ComponentProvider#362

Open
phillipc wants to merge 2 commits intomainfrom
fix/component-provider-parser-globals
Open

Fix Parser instantiation and globals in ComponentProvider#362
phillipc wants to merge 2 commits intomainfrom
fix/component-provider-parser-globals

Conversation

@phillipc
Copy link
Copy Markdown
Member

Summary

ComponentProvider.getComponentParams was instantiating Parser with a downcast (new (Parser as any)(node, context, this.globals)) and passing undefined as globals to .parse(). Since Parser takes no constructor arguments, the globals were silently dropped — global variables could never resolve in component params.

Changes

  • ComponentProvider.ts: Replace new (Parser as any)(node, context, this.globals) as Parser with new Parser(), and pass this.globals to .parse() instead of undefined. This matches how every other provider (BindingStringProvider, mustacheParser) uses Parser.
  • componentProviderBehaviors.ts: Add test that registers a component with params="answer: GLOBAL_CONST" where GLOBAL_CONST is provided via provider globals, verifying globals resolution in component params.

Verification

All 52 existing tests pass (chromium + happy-dom). New test passes in both environments.

Remove `as any` downcast on Parser constructor. Parser takes no
constructor arguments; context, globals, and node are parameters of
`.parse()`. The old code silently dropped `this.globals` (passed to
the no-op constructor) and sent `undefined` to `.parse()`, so global
variables could not resolve in component params.

Add test that exercises globals resolution through component params.

Adversarial review: subagent audit — no backwards-compat, disposal,
perf, or API surface concerns; fix aligns with every other provider.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 22, 2026

Warning

Rate limit exceeded

@phillipc has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 26 minutes and 14 seconds before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 75fa124d-a13f-47e5-9770-caa416b59890

📥 Commits

Reviewing files that changed from the base of the PR and between f670aa8 and 5452efc.

📒 Files selected for processing (4)
  • .changeset/fix-component-provider-parser-globals.md
  • packages/provider.component/spec/componentProviderBehaviors.ts
  • packages/provider.component/src/ComponentProvider.ts
  • packages/utils.parser/src/Parser.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/component-provider-parser-globals

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@brianmhunt
Copy link
Copy Markdown
Member

Adversarial review

Fix matches canonical usage in BindingStringProvider.ts:53 and mustacheParser.ts:20. New test would have failed pre-fix (globals defaulted to {}, GLOBAL_CONST resolves to undefined, expect(42) fails).

Must fix before merge:

  1. Changeset — none in diff. @tko/provider.component is a user-visible bug fix and the post-chore(release): collapse release flow to single human action #377 release pipeline won't bump versions without one.

Worth fixing while here:
2. Stale Parser JSDocpackages/utils.parser/src/Parser.ts:34-40 documents new Parser(node, context) with node/context/globals parameters. The class has no constructor; the doc has been a lie since the constructor was removed and is exactly what tricked the original ComponentProvider author into the as any downcast. Cheap to delete the parameter list while you're touching this surface.

Out of scope (note for the record):

  • Test doesn't unregister the xenon component or restore options.bindingProviderInstance — consistent with every sibling test in this file. Pre-existing pattern.

@brianmhunt
Copy link
Copy Markdown
Member

Changeset how-to (sorry, should have included this):

bunx changeset add
# select @tko/provider.component, pick "patch", paste short description
# commit the generated .changeset/<slug>.md with the fix

Or hand-write .changeset/fix-component-provider-globals.md:

---
'@tko/provider.component': patch
---

Fix Parser instantiation and globals in ComponentProvider — globals were silently dropped from component params resolution.

@phillipc
Copy link
Copy Markdown
Member Author

phillipc commented May 5, 2026

@brianmhunt done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants