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: Expose more AST types from "svelte/compiler" #14601

Merged
merged 14 commits into from
Dec 12, 2024

Conversation

xeho91
Copy link
Contributor

@xeho91 xeho91 commented Dec 7, 2024

So, given that in the Advent of Svelte you have created some new features to Svelte syntax.
Such as <svelte:boundary> and as in {#each} block is no longer needed.

So, I thought it's time to upgrade svelte-ast-print to support printing them too!

Firstly, I noticed that type SvelteBoundary was not a part of ElementLike type. Hence I was getting type error (any) when attempting to walk on it using zimmerframe.

In the meantime, I thought of taking an opportunity to export more types, which I had to manually provide in following places:

So, I've moved those types inside the AST namespace.
In the meantime I've updaded the codebase where those types are used, with this change - by using AST namespace.

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.
  • If this PR changes code within packages/svelte/src, add a changeset (npx changeset).

Tests and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

Copy link

changeset-bot bot commented Dec 7, 2024

🦋 Changeset detected

Latest commit: 7971acf

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte Minor

Not sure what this means? Click here to learn what changesets are.

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

Copy link
Contributor

github-actions bot commented Dec 7, 2024

Playground

pnpm add https://pkg.pr.new/svelte@14601

@Rich-Harris
Copy link
Member

preview: https://svelte-dev-git-preview-svelte-14601-svelte.vercel.app/

this is an automated message

@xeho91
Copy link
Contributor Author

xeho91 commented Dec 7, 2024

In the meantime, I have a small proposal to add another type:

type AttributeLike = Attribute | SpreadAttribute | Directive;

EDIT. I went ahead and added it.
Commit to revert in case: 4e0d43c

@xeho91
Copy link
Contributor Author

xeho91 commented Dec 7, 2024

And also, I have another issue: namespace Css and its types are not exported to the public.

I am not sure which one is the best solution:

  1. export to public as separate namespace,
  2. or export it inside AST namespace?

EDIT. I went ahead with the 2nd option.
Comit to revert in case: 24a25de

@Rich-Harris
Copy link
Member

Thank you! At a high level I think this makes sense. SvelteNode and TemplateNode are somewhat messy though, there are places where they should probably be tightened up (e.g. we should have a type that means 'can be a child of a fragment', i.e. Text | Tag | ElementLike | Block | Comment, that we can use in some of the places where we currently use TemplateNode. Not sure what such a thing should be called). Perhaps we could omit those in the meantime?

I also think we should probably Css to CSS, since we currently capitalise AST.

@xeho91
Copy link
Contributor Author

xeho91 commented Dec 10, 2024

I also think we should probably Css to CSS, since we currently capitalise AST.

I agree with this sentiment 👍

I wasn't sure if you meant:

  1. only to export namespace Css as CSS: 3b42438
  2. or rename it everywhere as AST.CSS: e185e1a
    Note that there's already an existing namespace CSS from TypeScript lib - "dom". Hence why internally named as _CSS, then in codebase used as AST.CSS.

@xeho91
Copy link
Contributor Author

xeho91 commented Dec 10, 2024

Thank you! At a high level I think this makes sense. SvelteNode and TemplateNode are somewhat messy though, there are places where they should probably be tightened up (e.g. we should have a type that means 'can be a child of a fragment', i.e. Text | Tag | ElementLike | Block | Comment, that we can use in some of the places where we currently use TemplateNode. Not sure what such a thing should be called). Perhaps we could omit those in the meantime?

Hm... as an developer using those types at higher level... let me demonstrate with a simple reproduction: https://stackblitz.com/edit/stackblitz-starters-evnjpuos?file=index.js

Without SvelteNode, I'm losing DX in terms of typing experience without SvelteNode. Do you have a suggestion of what I could use instead - to still have a nice typing experience?

@Ocean-OS
Copy link
Contributor

I wasn't sure if you meant:

  1. only to export namespace Css as CSS: 3b42438
  2. or rename it everywhere as AST.CSS: e185e1a
    Note that there's already an existing namespace CSS from TypeScript lib - "dom". Hence why internally named as _CSS, then in codebase used as AST.CSS.

It'd probably be best to rename it as AST.CSS, since it is the CSS AST node types.

@Rich-Harris
Copy link
Member

AST.CSS, yeah.

Without SvelteNode, I'm losing DX in terms of typing experience without SvelteNode. Do you have a suggestion of what I could use instead - to still have a nice typing experience?

That's true, it is useful to have a union of all the types you might encounter while walking. I did wonder about dividing things up more neatly so that in addition to AST.CSS there's AST.JS (containing Node from estree plus any TS-specific additions) and AST.Template... but it's all probably overkill. So yeah, maybe it's simpler to keep stuff as-is

@Rich-Harris Rich-Harris merged commit 61a0da8 into sveltejs:main Dec 12, 2024
10 checks passed
@github-actions github-actions bot mentioned this pull request Dec 12, 2024
@xeho91 xeho91 deleted the ast-types branch December 13, 2024 03:21
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