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

Auto Imports, remove JS/TS config #677

Merged
merged 25 commits into from
Jan 18, 2024
Merged

Conversation

machty
Copy link
Contributor

@machty machty commented Jan 11, 2024

See #130

Basic Support for Auto-Import (for all supported filetypes, including .gts)

You are now offered code completions for auto-imports, and selecting them will add the import at the top of the file.

Remove JS/TS configuration options

Previously we were attempting to read the JS/TS VScode configuration preferences in order to pass them through to the VS LSP within Glint, but since the goal is feature parity with the TS LSP such that you can (and should) disable the default TS LSP, we can't continue to use the default JS/TS config options because they get disabled then you disable the TS built-in extension. This PR replaces them with sensible hard-wired defaults, and we can consider adding configurable VS code preferences specific to Glint in a future PR.

Add localhost dev/debug instructions to CONTRIBUTING.md

This would have been very helpful for me getting up and running.

(Won't fix) Completions don't work on unclosed tags within <template>

Likely not fixable until we have some syntax-forgiving Glimmer parser.

@machty
Copy link
Contributor Author

machty commented Jan 11, 2024

cc @NullVoxPopuli @dfreeman @wagenet

@machty
Copy link
Contributor Author

machty commented Jan 11, 2024

Update: got auto-complete working in both .ts and .gts with default TS engine disabled.

image image

Going to spend some time unifying the text preview to make it clearer what kind of completion/import it is.

@machty
Copy link
Contributor Author

machty commented Jan 12, 2024

Pushed some updates that greatly improve the default formatting of auto imports.

Screenshot 2024-01-12 at 11 36 24 AM Screenshot 2024-01-12 at 11 36 30 AM

@machty machty changed the title WIP: attempt to get auto imports working Support for auto imports, improved code completion previews Jan 12, 2024
@machty machty marked this pull request as ready for review January 12, 2024 12:26
@machty machty changed the title Support for auto imports, improved code completion previews Auto Imports, remove JS/TS config Jan 12, 2024
@machty
Copy link
Contributor Author

machty commented Jan 12, 2024

I think this PR is probably ready for a review. Here's how things ended up:

image

I do see one issue though:

image

For some reason the auto imports don't kick in depending on where you are in the tag... not sure if worth fixing in this PR or a separate issue.

@@ -17,3 +17,43 @@ Glint is a family of packages which all live in this repo as a Yarn workspace. T
- Read the project’s [ARCHITECTURE.md](./ARCHITECTURE.md) to understand the basics of how the code base works.

Once you have made changes and added tests to confirm they work correctly, you can then open a PR and we'll work with you to polish it up and get it landed!

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks a ton for adding this! This is all the information i got super stuck on not having when i was first getting in to glint!

Very needed!

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks a ton for adding tests!!

@wagenet wagenet merged commit 7bf48d4 into typed-ember:main Jan 18, 2024
6 checks passed
@spuxx1701
Copy link

This is great! Thanks for adding this <3

@machty machty deleted the machty-auto-import branch March 21, 2024 08:16
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.

4 participants