Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: capture render tree can fail if args cannot be obtained #1447

Merged
merged 2 commits into from
Oct 23, 2023

Conversation

patricklx
Copy link
Contributor

this currently happens on https://meta.discourse.org/ when checking with ember-inspector and not being logged in.

this currently happens on https://meta.discourse.org/
when checking with ember-inspector and not being logged in.
@NullVoxPopuli
Copy link
Contributor

hello! can a test be added? thanks!

@patricklx
Copy link
Contributor Author

Hi, where can I add that?

@NullVoxPopuli
Copy link
Contributor

maybe in here? https://github.com/glimmerjs/glimmer-vm/blob/master/packages/%40glimmer-workspace/integration-tests/test/debug-render-tree-test.ts

@patricklx
Copy link
Contributor Author

added the tests for this.
Could this also be backported?

@patricklx
Copy link
Contributor Author

@NullVoxPopuli

@NullVoxPopuli
Copy link
Contributor

unsure, what version would you want to backport to?

@NullVoxPopuli
Copy link
Contributor

for others, a repro of the problem: https://stackblitz.com/edit/github-pgxsyq?file=app%2Fcomponents%2Fwelcome.gts

(open the ember inspector, ensure the throw is happening, and see that the component view is blank)

@patricklx
Copy link
Contributor Author

Backportel to lts versions would be nice i think

@NullVoxPopuli
Copy link
Contributor

glimmer-vm doesn't have an LTS tho 😅

an issue is that even if we fix ember-LTS, all the versions in between won't be fixed, because glimmer-vm is bundled with ember-source right now.

@NullVoxPopuli NullVoxPopuli merged commit 8caba05 into glimmerjs:master Oct 23, 2023
5 checks passed
@patricklx patricklx deleted the patch-3 branch October 23, 2023 21:04
@patricklx
Copy link
Contributor Author

patricklx commented Oct 24, 2023

@NullVoxPopuli i think we need to revert this.
it affects also other parts of glimmer...

@NullVoxPopuli
Copy link
Contributor

How do you mean? Can we add more tests to prevent whatever it in you've discovered?

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Oct 24, 2023

Looks like #1460 explains

@patricklx
Copy link
Contributor Author

for example here it is used: https://github.com/glimmerjs/glimmer-vm/blob/master/packages/%40glimmer/runtime/lib/helpers/fn.ts#L87.
in application this change would have caught errors, and the developer would not see it in the app...

@NullVoxPopuli
Copy link
Contributor

Oofta, that tells me we don't have sufficient failure-case testing

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants