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

fix: rework directive name handling #9470

Merged
merged 3 commits into from
Nov 15, 2023
Merged

fix: rework directive name handling #9470

merged 3 commits into from
Nov 15, 2023

Conversation

Rich-Harris
Copy link
Member

Unfortunately #9462 introduced a bug (cc @paoloricciuti and @dummdidumm) — a directive like use:a.b.c-d needs to become get(a).b['c-d'] if a is state (mutatis mutandis if it's a prop).

This was surfaced by TypeScript as soon as I rewrote member_expression_id_to_literal to return an expression instead of a string (mostly because I wanted a.b.c-d to result in a.b['c-d'] rather than a['b']['c-d'], because serialize_get_binding will only accept an Identifier.

I also got rid of the snapshot test. We should use these extremely sparingly — most of the time we don't care that much about the precise nature of the generated code, we only care about whether the generated code is valid. Snapshot tests are annoying because they frequently need to be updated for reasons unrelated to the test itself.

I'm not sure I agree with the decision in #9462 to do this at transform time rather than parse time. For the sake of things like sourcemap generation, it's good to preserve as much information about node start/end as possible, and it means we can apply this logic in a single place. I haven't done that yet though, we should discuss.

Finally, by working through this problem I think I've uncovered a separate bug — animate/transition directives 'lock on' to the directive value at component init time, even though it may change by the time the directive takes effect. It's less broken than it is on main, but it's still broken. Will create a follow-up issue/PR for that once this is merged.

Copy link

changeset-bot bot commented Nov 15, 2023

⚠️ No Changeset found

Latest commit: becfcbc

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@paoloricciuti
Copy link
Member

Uh I didn't realized that sorry

Copy link

vercel bot commented Nov 15, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
svelte-5-preview ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 15, 2023 4:11pm

@Rich-Harris
Copy link
Member Author

Not at all! None of this stuff is all that obvious, I was just explicitly flagging it so it didn't seem like i was passively-aggressively rewriting code for no reason :)

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

The reason I was against adding it to the AST is that for example prettier relies on these fields for printing, because they're verbatim, so no need to extract them from the original source

@Rich-Harris Rich-Harris merged commit d749685 into main Nov 15, 2023
8 checks passed
@Rich-Harris Rich-Harris deleted the rewrite-snapshot-test branch November 15, 2023 17:52
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.

3 participants