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

[feat] improve parent_component undefined error #6991

Closed
wants to merge 5 commits into from

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Dec 6, 2021

Fixes #6584

If target option is not defined, ensure current_component is defined too, which is used in component init() phase.

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with [feat], [fix], [chore], or [docs].
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with npm test and lint the project with npm run lint

@bluwy
Copy link
Member Author

bluwy commented Dec 6, 2021

Actually I just realized something, the error message would only apply to the user's project's own components only, and in most case they shouldn't hit this issue. The core issue is that other libraries precompile into JS, and the error arise in that precompiled code. So that makes this PR not very useful most of the time. though maybe useful for svelte-jester.

The real way to detect the error is by wrapping a try catch when initializing the precompiled JS component and re-transform the error, which is a bit hacky.

@dummdidumm
Copy link
Member

Oh shit, yeah I guess you're right ..

@bluwy
Copy link
Member Author

bluwy commented Dec 6, 2021

Looking at svelte-jester's source, looks like this PR would help it surface the better error message. But other than that, I don't see any other usecases.

I could add the try...catch technique to capture invalid precompiled components usage, but I'm not sure if that's fine perf-wise. But if it's ok, I can make that change here. Otherwise I don't think this PR closes #6584.

@dummdidumm
Copy link
Member

If it's possible to add this as a dev-only warning I'd say this would be a good addition. What did you have in mind to implement this?

@bluwy
Copy link
Member Author

bluwy commented Dec 7, 2021

It'll most likely be some sort of wrapper function when instantiating a Svelte component in the compiled code. Something like: init_component_dev(Foo) instead of new Foo({}). And we'll have some heuristics in place when it throws. I'll try to roughly implement this soon so marking as draft for now.

Update: Or maybe I can re extend the component class, and wrap the checks there. This sounds more robust.

@bluwy bluwy marked this pull request as draft December 7, 2021 08:41
@bluwy
Copy link
Member Author

bluwy commented Jan 25, 2022

Tried implementing the try-catch solution today, but I can't find a solid heuristic to detect this edge case, especially when the code is minified. The only way I thought of is to have the SvelteComponent class expose a get_current_component method, so we could compare it, but that requires the incorrectly bundled Svelte library to upgrade their Svelte version before we can do so, which doesn't sound great. Closing this for now, perhaps better documentation is needed.

@bluwy bluwy closed this Jan 25, 2022
@dummdidumm
Copy link
Member

Could you show what you had for the try-catch solution? Maybe we can come up with something that works for most cases from that.

@bluwy
Copy link
Member Author

bluwy commented Jan 26, 2022

Basically whenever we generate code that instantiates the component, like:

function create_fragment(ctx) {
  let foo
  foo = new Foo({})
}

I was thinking we can have something like:

function create_fragment(ctx) {
  let foo
  foo = validate_component(() => new Foo({}))
}

Which wraps the function call in a try catch to detect the error. I also tried inline extending the Foo component, but I found that doesn't make sense as the separate Svelte instance is within the compiled Foo code, and extending it can't exactly guard it.

The issue with try catch is that the error message varies between Chrome, Firefox, and Safari. Plus the error message won't always be Cannot read property '$$' of null if the code was minified.

@dummdidumm
Copy link
Member

In which cases this could be an error that is unrelated? Would it be feasible to always append additional text like "if the instantiated component is precompiled then .."?

@bluwy
Copy link
Member Author

bluwy commented Jan 26, 2022

In which cases this could be an error that is unrelated?

Any error could come up during the component initialization phase. Given this example, if we have Foo as an external bundled component, the init(..., ..., instance) call in the constructor would invoke instance immediately, which could bring up unrelated errors.

Would it be feasible to always append additional text like "if the instantiated component is precompiled then .."?

I'm not sure if it's fine to alter the error message. It might be confusing, but otherwise I don't see any other safe way.

@benmccann benmccann deleted the improve-parent-component-errror branch August 22, 2023 22:06
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.

New component root property may throw errors
2 participants