Skip to content

feat: disallow calling getContext inside element #7864

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

Closed
wants to merge 2 commits into from

Conversation

tanhauhau
Copy link
Member

Fixes #7549

Throwing error when using getContext inside element, eg: attribute, text content, ...

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

  • [ x] 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

@baseballyama
Copy link
Member

I think CI is actually failed.

@tanhauhau
Copy link
Member Author

oh okay i'll take a look at it 🙈

@benmccann benmccann changed the title [feat] disallow calling getContext inside element feat: disallow calling getContext inside element Jan 12, 2023
@dummdidumm dummdidumm added this to the 4.x milestone Feb 22, 2023
@dummdidumm
Copy link
Member

marked this as v4 because strictly speaking it's a breaking change

@vercel
Copy link

vercel bot commented Apr 11, 2023

@dummdidumm is attempting to deploy a commit to the Svelte Team on Vercel.

A member of the Team first needs to authorize it.

@dummdidumm dummdidumm changed the base branch from master to version-4 April 11, 2023 13:38
@benmccann
Copy link
Member

@tanhauhau if you get a chance, it looks like the tests will need to be updated on this one

@dummdidumm
Copy link
Member

Wondering if we should just leave it be and not do this explicitly, instead stating in the docs that using getContext inside the markup is undefined behavior. That would free us from having to keep that check for Svelte 5, which may or may not be hard.

@benmccann
Copy link
Member

If we added the check now, I don't think we'd be obligated to keep if for Svelte 5. For Svelte 4, it seems a bit safer than a warning the docs that people may or may not see.

Copy link
Member

@baseballyama baseballyama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this PR breaks {#await} block.

<script>
  let promise = Promise.resolve();
</script>

{#await promise}
  <p>waiting...</p>
{:then value}
  <p>then!</p>
{/await}

image

To fix this, I think we need variables scope analyzer and throw compile error if get_context is used in the template block.
But handling this by Svelte compiler is a bit difficult because we don't have a scope analyzer.

So to do this, an easier way is that we will implement this in ESLint plugin.

What do you think?

@dummdidumm dummdidumm modified the milestones: 4.x, one day Apr 26, 2023
@dummdidumm
Copy link
Member

Given that it looks to be really tricky to get this working properly I'm giving this the "one day" label. I think a note in the docs that you shouldn't use Svelte functions in the template would be enough.

@benmccann benmccann deleted the branch sveltejs:svelte-4 June 20, 2023 20:44
@benmccann benmccann closed this Jun 20, 2023
@benmccann benmccann reopened this Jun 20, 2023
@benmccann benmccann changed the base branch from version-4 to master June 20, 2023 21:36
@Rich-Harris Rich-Harris modified the milestones: one day, 5.0 Apr 1, 2024
@Rich-Harris
Copy link
Member

Closing since we ended up going in the opposite direction: #11061

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

Successfully merging this pull request may close these issues.

getContext is undefined when called inside an element attribute.
5 participants