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 instead of ember-template-imports #655

Merged
merged 4 commits into from
Mar 12, 2024

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Nov 30, 2023

ember-template-imports @ v4 removed all the APIs that glint was relying on for working with <template>-tags

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

Unblocks: NullVoxPopuli/polaris-starter#11
Supersedes: #615

Extractions:

Allows:

  • folks to <pm> add ember-template-imports in their apps and not have to downgrade to v3 if they want to use Glint
  • better syntax handling
  • better incorrect syntax handling


// TODO: is there a way to convert bytes to chars?
// I think maybe all of this code needs to live in content-tag,
// and give us the option to generate this sort of structure
templateLocations.push({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is all likely wrong, as the original code used character-indices

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still likely wrong?

Copy link
Contributor Author

@NullVoxPopuli NullVoxPopuli Mar 11, 2024

Choose a reason for hiding this comment

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

yeah, char positions are expected, but I only have bytes here, so whenever there is a multi-byte character, where the errors show up may be offset

Copy link
Contributor

@patricklx patricklx Mar 11, 2024

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah ok, I'll try that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pushed!

this is looking excellent!

image
image
image

@@ -6461,7 +6466,7 @@ ember-template-imports@^1.1.1:
ember-cli-version-checker "^5.1.2"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

above here, more reason to have glint not depend on content-tag:

=> Found "[email protected]"
info Reasons this module exists
   - "_project_#@glint#environment-glimmerx#@glimmerx#component" depends on it
   - Hoisted from "_project_#@glint#environment-glimmerx#@glimmerx#component#ember-template-imports"

yarn.lock Outdated
@@ -6461,7 +6466,7 @@ ember-template-imports@^1.1.1:
ember-cli-version-checker "^5.1.2"
validate-peer-dependencies "^1.1.0"

ember-template-imports@^3.0.0, ember-template-imports@^3.1.1:
ember-template-imports@^3.1.1:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This'll soon be removed from ember-template lint as well:

=> Found "ember-template-lint#[email protected]"
info This module exists because "_project_#ts-ember-app#ember-template-lint" depends on it.
info Disk size without dependencies: "232KB"

@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review December 18, 2023 22:53
@NullVoxPopuli
Copy link
Contributor Author

I'm marking as ready for review, as I think this is about at the point, where I want to make sure I'm not doing anything crazy.

There is still a failing test: https://github.com/typed-ember/glint/actions/runs/7254626609/job/19763687113?pr=655#step:6:107
which... I have no idea how this code affects the reporting of untyped {{component}} usage, since in the "debug extension" thing, the 4 use cases all error correctly (as expected).

cc @dfreeman @wagenet @gitKrystan

Some things left for this PR tho:

} catch (e) {
if (typeof e === 'object' && e !== null) {
// Parse Errors from the rust parser
if ('source_code' in e) {
Copy link
Contributor Author

@NullVoxPopuli NullVoxPopuli Dec 18, 2023

Choose a reason for hiding this comment

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

It may be useful for content-tag to emit this error as a JSON object or something (maybe a property on the already emitted error?) so I don't have to parse the error-string here.

//
// https://github.com/embroider-build/content-tag/blob/v1.2.2-content-tag/package.json#L13-L21
//
// @ts-expect-error see above
Copy link
Contributor Author

Choose a reason for hiding this comment

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

even though this ts-expect-error is here, we still get the correct type for Preprocessor and p.parse below

Copy link
Member

Choose a reason for hiding this comment

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

Interesting. I wonder why I don't get this in the prettier plugin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is it using 1.2.2 / same tsconfig settings from glint?

let sourceOffsetBytes = 0;
let deltaBytes = 0;

// @ts-expect-error TS couldn't find @types/node, which are specified
Copy link
Contributor Author

Choose a reason for hiding this comment

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

idk why this couldn't be found

let endTagLength = template.end[0].length;
let startTagOffset = start.index ?? -1;
let endTagOffset = end.index ?? -1;
let startTagLengthBytes = template.startRange.end - template.startRange.start;
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 added Bytes to all the variables here as a suffix to keep the units straight.

See also: embroider-build/content-tag#45

* SEE: https://github.com/embroider-build/content-tag/issues/39#issuecomment-1832443310
*/
let prefixingSegment = sourceBuffer.slice(sourceOffsetBytes, startTagOffsetBytes);
segments.push(prefixingSegment.toString());
Copy link
Contributor Author

@NullVoxPopuli NullVoxPopuli Dec 18, 2023

Choose a reason for hiding this comment

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

is there a way to not do this segments stuff?

@dfreeman since content-tag can transform to valid JS, could we just parse that instead?

I believe this is what @patricklx may have run in to in the past as well while iterating on the eslint parser, we need a pre-transform to post-transform index mapping for all AST nodes


// @ts-expect-error TS couldn't find @types/node, which are specified
// in the root package.json
let sourceBuffer = Buffer.from(source);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See https://runkit.com/nullvoxpopuli/content-tag-byte-vs-char-offsets for a brief overview of how Buffer solves the byte-index vs char-index issue.

(As well as embroider-build/content-tag#45 )

@lifeart
Copy link

lifeart commented Jan 23, 2024

I like this changes! We definitely need to push it forward and land it! Works just fine inside custom glint env https://github.com/lifeart/glimmer-next/tree/master/glint-environment-gxt

Copy link
Contributor

@wagenet wagenet left a comment

Choose a reason for hiding this comment

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

I think this is ok. I still don't have 100% context.


if (startTagOffset === -1 || endTagOffset === -1) continue;
// if (startTagOffset === -1 || endTagOffset === -1) continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, can do


// TODO: is there a way to convert bytes to chars?
// I think maybe all of this code needs to live in content-tag,
// and give us the option to generate this sort of structure
templateLocations.push({
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still likely wrong?

@@ -11,6 +11,6 @@
"composite": true,
"declaration": true,
"sourceMap": true,
"types": []
"types": ["@types/node"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this change was needed so that the types for Buffer existed

{ "path": "packages/vscode" },
{ "path": "packages/scripts" },
{ "path": "test-packages/test-utils" }
{ "path": "./packages/core" },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

in one of the commits, it's explained that this change was needed so that you can ctrl+click to go to each of these tsconfigs.

It's kinda silly that tsc works with both path-formats but vscode only works with leading ./

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants