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

dx(computed): warn incorrect use of inject or lifecycle hooks inside computed #10038

Closed
wants to merge 1 commit into from

Conversation

skirtles-code
Copy link
Contributor

This is a follow up to 324e817. That added a warning when getCurrentInstance is called from a computed getter.

That warning does cover several real cases, including useI18n and the Oruga problem in #10001. But in #9974 I mistakenly suggested that it would also cover misuse of inject or lifecycle hooks.

While those APIs do use the current instance, they don't access it via getCurrentInstance.

I've added similar warnings for both inject and lifecycle hooks. There are various other functions where warnings could also be added, but for this PR I stopped at these two. In practice, I suspect that'll cover most composables that aren't safe to use in this way.

One advantage of not using getCurrentInstance is that it allows the warnings to be a bit more specific. A warning that explicitly mentions inject is probably more helpful than a warning about the undocumented getCurrentInstance.

Note that for inject I also check for !currentApp. While I don't have a specific use case in mind, it seemed like it could be legitimate to call inject if runWithContext is wrapped around it, as that would guarantee the existence of a suitable injection context.

Copy link

github-actions bot commented Jan 8, 2024

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 89.5 kB 34.1 kB 30.7 kB
vue.global.prod.js 146 kB 53.3 kB 47.6 kB

Usages

Name Size Gzip Brotli
createApp 49.8 kB 19.5 kB 17.8 kB
createSSRApp 53.1 kB 20.8 kB 19 kB
defineCustomElement 52 kB 20.2 kB 18.5 kB
overall 63.3 kB 24.4 kB 22.2 kB

@jacekkarczmarczyk
Copy link
Contributor

Shouldn't computed setter be checked as well?

@skirtles-code
Copy link
Contributor Author

I've switched this to a draft while the conversation in #9974 is ongoing. It seems unlikely that this can be merged in its present form, but some parts of it (such as just warning for lifecycle hooks) may be salvageable.

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.

None yet

2 participants