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

Use content tag #615

Closed
wants to merge 4 commits into from
Closed

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Aug 29, 2023

Resolves: #609
Blocked by: embroider-build/content-tag#19
Blocked by: embroider-build/content-tag#31

Unblocks: NullVoxPopuli/polaris-starter#11

Problem:

  • content-tag requires syntax to be correct
  • LSPs need fault-tolerant parsing to aid in intellisense, suggestions and all that

This isn't a new problem for Glint, but it does take things in the wrong direction a bit.

Copy link
Member

@dfreeman dfreeman left a comment

Choose a reason for hiding this comment

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

  • content-tag requires syntax to be correct
  • LSPs need fault-tolerant parsing to aid in intellisense, suggestions and all that

This isn't a new problem for Glint, but it does take things in the wrong direction a bit.

Based on Ed's research yesterday, it sounds like this isn't quite true, which is great news!

import { Preprocessor } from 'content-tag';
const p = new Preprocessor();

interface Parsed {
Copy link
Member

Choose a reason for hiding this comment

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

Can we have content-tag expose this type itself, either by generating it from the underlying Rust struct(s) or something like #[wasm_bindgen(typescript_custom_section)]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so -- tho I don't know how specific we'd get. @ef4 said an update to one of the build tools would give us better types when updated.

It's also totally reasonable for us to ship our own d.ts, and ignore the generated one.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not too hung up on any specific approach on the content-tag side; it just makes me nervous to be encoding assumptions here that could break at any time 😅

@NullVoxPopuli
Copy link
Contributor Author

NullVoxPopuli commented Oct 5, 2023

Observations:

Deleting the closing </template>

Example 1

kinda goofy
image

Before this PR, the experience is worse
image

Example 2

this actually makes sense and provides meaningful feedback about the missing closing </template>, which I believe is an improvement?
image

Before this PR, the experience is worse
image

Deleting the opening <template>

Example 1

sqigglies everywhere.
maybe this is something we can get our swc parser to help with though?

image

Before this PR, the experience is not helpful, but 🤷 when it comes to deciding if better or worse, imo
image

Deliberate syntax error

"importing a number"

image

Before this PR, the experience is better
image

extra quote

image

Before this PR, the experience is better
image

number * string

image

Before this PR, the experience is better
image

forgetting to give a variable a value

image

Before this PR, the experience is better
image

fun thing(s)

accidentally discovered function noop shorthand

this is actually valid, but looks goofy

image

@NullVoxPopuli
Copy link
Contributor Author

I broke something here, so I'm starting over here: #655

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.

Use content-tag in @glint/environment-ember-template-imports
2 participants