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: typescript #90

Closed
wants to merge 5 commits into from
Closed

Conversation

manuel3108
Copy link
Contributor

Closes #86
Relates svelte-add/svelte-add#507
Relates svelte-add/svelte-add#193

I wanted to see if we can get any closer to supporting typescript, while using the tooling we are currently using in svelte-add. And this comes surprisingly close, with "only" 12 tests failing.

Of course there are a few things I didn't bother about right now:

  • There are a few formatting differences between esrap and recast, especially regarding quotes and lines breaks (11 failed tests)
  • Exceptions (1 test)

Otherwise the typescript test should be working as expected:
image

This is by no means a full implementation, and we should probably add more tests to see if this works as expected, even inside html templates potentially. This is a minimal implementation, that's why I did not bother renaming print_es

On other notes:

  • Linting script seems to be off, although main seems to be passing (produces to many errors locally to display them all in the console)
  • Pipeline is reporting three snapshot failures but is still passing (https://github.com/xeho91/svelte-ast-print/actions/runs/9885196416/job/27302793808)
  • That's maybe on me: vscode tries to format every file completely differently as it's in the repository. I had to save my stuff "without formatting" to produce this minimal diff.

Copy link

socket-security bot commented Jul 27, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher

View full report↗︎

@manuel3108
Copy link
Contributor Author

Actually this is not going to work as I expected, as recast is printing all object properties into new lines, even if they are below the row printWidth. Here is the PR for that benjamn/recast#347 although it's already 8 years old.

So my chosen approach might not work as initially expected.

@benmccann
Copy link

Here is the PR for that benjamn/recast#347 although it's already 8 years old.

Bummer that the author never finished it up. But the maintainers liked the PR and the repo is still actively maintained, so if we wanted to clean it up and send a new version it sounds like they would accept it.

@manuel3108
Copy link
Contributor Author

I just patched the multiline behavior (ugly for now, but works), which brings us down to 6 failing tests. But then i discoverd other recast flaws, so I don't think continuing this way would result in results fast. Here are a few of the flaws:

Comments get removed
image

Strange brackets removal / strange new line
image

Inconsistent indendation (even tho useTabs was set to true)
image

@xeho91
Copy link
Owner

xeho91 commented Jul 29, 2024

Hi!

Let me start with most important message: thank you for contributing! ❤️

I wanted to let you know that I've seen your efforts on mobile. Tomorrow I'll have access to my laptop and I will catch up on your notes and help in any way I can.

@xeho91
Copy link
Owner

xeho91 commented Jul 31, 2024

  • Linting script seems to be off, although main seems to be passing (produces to many errors locally to display them all in the console)

That's troubling! Do you mind sharing at least the first error messages? I'm curious which one of linter(s) is failing.

Ah yes, this may be counter-intuitive or confusing. I'm aware of these 3 snapshots failing, because TypeScript syntax isn't supported yet. I've made these tests for future implementation and marked with it.fails, so using running tests would return a 0 process exit code (success).
Like, I am telling "Hey Vitest, I know this will fail. I acknowledge it, but please don't block my pipeline with automatic release workflow".

That's maybe on me: vscode tries to format every file completely differently as it's in the repository. I had to save my stuff "without formatting" to produce this minimal diff.

Are you using Prettier or Biome? This repository uses Biome.

@xeho91
Copy link
Owner

xeho91 commented Jul 31, 2024

  • There are a few formatting differences between esrap and recast, especially regarding quotes and lines breaks (11 failed tests)
  • Exceptions (1 test)

Otherwise the typescript test should be working as expected: image

I want to assure that formatting isn't currently a priority, so we can worry less about it. Is ok to update tests snapshots if they're just related to output format. Consumers of this package can easily solve this by using Prettier or Biome to printed output code from AST nodes by this package.

Being able to support printing TypeScript AST nodes as soon as possible is the missing crucial feature.

@xeho91
Copy link
Owner

xeho91 commented Jul 31, 2024

Comments get removed image

That's a big blocker, I've tried to figure out if there was an option for recast' print which would prevent this from happening. Below is my quick research:

recast uses esprima for parsing. Hence based on the source code, you might need to pass option comments: true based on:

Strange brackets removal / strange new line image

Brackets removal is format related, no need to worry about this one for now.
As for new line, this feels odd. Need to verify if this is actually a valid syntax?

@manuel3108
Copy link
Contributor Author

That's troubling! Do you mind sharing at least the first error messages? I'm curious which one of linter(s) is failing.

Looking at the package.json for me it seemed logical that I need to run pnpm lint. I have attached you the full output.
lint.txt

Ah yes, this may be counter-intuitive or confusing. I'm aware of these 3 snapshots failing, because TypeScript syntax isn't supported yet. I've made these tests for future implementation and marked with it.fails, so using running tests would return a 0 process exit code (success).
Like, I am telling "Hey Vitest, I know this will fail. I acknowledge it, but please don't block my pipeline with automatic release workflow".

Ahh, yes I have seen that previously. Just wondering why it's printing it in red in this case, but then everything seems to be fine.

Are you using Prettier or Biome? This repository uses Biome.

That's probably the right clue. Didn't think of that but makes sense.

I want to assure that formatting isn't currently a priority, so we can worry less about it. Is ok to update tests snapshots if they're just related to output format. Consumers of this package can easily solve this by using Prettier or Biome to printed output code from AST nodes by this package.

Being able to support printing TypeScript AST nodes as soon as possible is the missing crucial feature.

I have just updated some snapshots that were only formatting related and not such big deals. That brings us down to 4 fails

  • 1 exception
  • 2 comment removals
  • 1 new line after $: syntax

Will see if we can get closer to zero without too much trouble.

That's a big blocker, I've tried to figure out if there was an option for recast' print which would prevent this from happening. Below is my quick research:

recast uses esprima for parsing. Hence based on the source code, you might need to pass option comments: true based on:

Don't fully agree with that. As we are already passing the nodes directly to the print function there should any parsing or esprima stuff involved on the recast side. I'm assuming that it's just the printer that does not support comments right now. Will have a look at that.

As for new line, this feels odd. Need to verify if this is actually a valid syntax?

If we trust the repl, this seems to be valid. But I would still like to avoid this: https://svelte.dev/repl/2e818715bffa45a69d6f6753561da287?version=4.2.18

@xeho91
Copy link
Owner

xeho91 commented Aug 1, 2024

That's troubling! Do you mind sharing at least the first error messages? I'm curious which one of linter(s) is failing.

Looking at the package.json for me it seemed logical that I need to run pnpm lint. I have attached you the full output. lint.txt

So, it's Biome not being happy with the format. I admit I am a little confused by what is happening in your case. Because I can't reproduce those errors locally nor GitHub Actions workflows with CI setup does complain about it.

From the shared output in file I see that somehow carriage return () aka \r\n was added to those failing files, and Biome is suggesting them to replace them with new lines (\n). I suspect you're using Windows(?). I think your hunch was right that your VSCode is trying to format files by itself (see if they're added to changed files by Git).

There's a guide for integrating Biome with VSCode for reference. I personally use Neovim, so I'm not fluent with VSCode setup. I don't want to enforce any settings on you; you know better what works best for you. Although I have to admit that the default(?) behaviour of auto-formatting files (in the open workspace/repository) which are not being currently changed by you, feels uncommon and too invasive to me.

@manuel3108
Copy link
Contributor Author

Sadly I have to come to the conclusion, that this is not possible at this point. recast handles comment completely differently than all AST's do. After parsing a string to an AST via esprima or other tools, they do some ast manipulation magic (that I dont understand) to add the comments to other nodes and handle edge cases. As we do not use the recast parser this does not apply to our ast and comments are not printed.

I tried re-creating that logic in other ways, but sadly without success. With all the limitations we have encountered here, I don't think this is a good way anymore and we should instead focus on Rich-Harris/esrap#10

@manuel3108 manuel3108 closed this Aug 7, 2024
@xeho91
Copy link
Owner

xeho91 commented Aug 8, 2024

Sadly I have to come to the conclusion, that this is not possible at this point. recast handles comment completely differently than all AST's do. After parsing a string to an AST via esprima or other tools, they do some ast manipulation magic (that I dont understand) to add the comments to other nodes and handle edge cases. As we do not use the recast parser this does not apply to our ast and comments are not printed.

I tried re-creating that logic in other ways, but sadly without success. With all the limitations we have encountered here, I don't think this is a good way anymore and we should instead focus on Rich-Harris/esrap#10

Thank you for the update, and most importantly - thank you for trying!

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.

Add support for TypeScript
3 participants