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

TSX Sourcemaps #518

Merged
merged 47 commits into from
Oct 3, 2022
Merged

TSX Sourcemaps #518

merged 47 commits into from
Oct 3, 2022

Conversation

natemoo-re
Copy link
Member

Changes

  • Gets convertToTSX sourcemaps working
  • Adds test coverage for TSX sourcemaps
  • Lays the foundation for better JS sourcemaps/test coverage
  • Supercedes Add tests for TSX sourcemaps #462

Testing

Lots of tests added

Docs

N/A

@changeset-bot
Copy link

changeset-bot bot commented Sep 22, 2022

🦋 Changeset detected

Latest commit: ae14f48

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

This PR includes changesets to release 1 package
Name Type
@astrojs/compiler 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

@natemoo-re natemoo-re marked this pull request as draft September 22, 2022 15:47
@natemoo-re natemoo-re marked this pull request as ready for review September 27, 2022 14:16
@Princesseuh
Copy link
Member

Princesseuh commented Sep 27, 2022

Living feedback document here:

Source map issues

  • Everything seems to be off by one
  • The source parameter is always ['<object>'] instead of the actual file information, this makes generatedPositionFor calls fails when they're done using the proper file info (possibly same problem as the second output issue?)

Output issues

  • Invalid markup should be left as is, to support completions on incomplete tags
  • Filenames don't seems to be processed correctly, no matter what I pass, I get the same result: object__AstroComponent_

Misc

  • It'd be great if the types were exported (TSXResult). Also it seems like the type is wrong, map is an object not a string

@FredKSchott
Copy link
Member

I'll leave this for @Princesseuh and you to review, just saying how exciting it is to see this PR!

Copy link
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

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

lgtm, nothing blocking but a couple of comments. nice tests!

@natemoo-re natemoo-re linked an issue Sep 28, 2022 that may be closed by this pull request
@jasikpark
Copy link
Contributor

should packages/compiler/deno/astro.wasm be .gitignore'd?

@natemoo-re
Copy link
Member Author

Yeah good call @jasikpark, we needed it for deploying the astro_compiler Deno package, but it's not maintained so we can probably drop it. Will do that in another PR.

@jasikpark
Copy link
Contributor

oh, I just meant that build artifacts should be git-ignored :p

do you mean that the wasm had to be commited to the git repo?

@natemoo-re
Copy link
Member Author

natemoo-re commented Sep 30, 2022

do you mean that the wasm had to be commited to the git repo?

Yep, to be included in the Deno package, that particular build artifact had to be committed. My guess is that the publishing workflow is more flexible than last time I tried, but we're not maintaining it, so no need to try to get it working.

Copy link
Member

@Princesseuh Princesseuh left a comment

Choose a reason for hiding this comment

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

Passes the language-server's test suite! 🥳

image

Remaining fixes (if there's any) can definitely be tackled in separate PRs. Awesome work Nate!

@matthewp
Copy link
Contributor

matthewp commented Oct 3, 2022

Wow, what a big effort, congrats @natemoo-re @Princesseuh

@natemoo-re natemoo-re merged commit 0be58ab into main Oct 3, 2022
@natemoo-re natemoo-re deleted the feat/tsx-sourcemaps branch October 3, 2022 16:00
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.

🐛 BUG: TSX output could type Astro.props automatically
5 participants