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.

Sign up for GitHub

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

Jump to bottom

use new glimmer ElementNode sub nodes #696

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

use new glimmer ElementNode sub nodes #696

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
eb16ae6
update glimmer
patricklx Jan 29, 2024
eb75445
fix
patricklx Jan 29, 2024
f0bf4c4
for component too
patricklx Jan 29, 2024
240b8d4
fix build
patricklx Jan 30, 2024
d9f6d21
fix lint
patricklx Jan 30, 2024
3be0379
fix tests
patricklx Jan 30, 2024
962ec06
fix lint
patricklx Jan 30, 2024
a6f4f07
ignore
patricklx Jan 30, 2024
ef094ec
ignore
patricklx Jan 30, 2024
15286b9
fix
patricklx Jan 30, 2024
bf1df59
revert
patricklx Jan 30, 2024
7103b7d
fix
patricklx Feb 1, 2024
3fedc71
We *have* to skipLibCheck, or else we can't build. We have to convert…
NullVoxPopuli Feb 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,12 @@
"@types/node": "Locking to avoid conflicts between the declared version in packages/core and floating '*' versions when we run in CI without the lockfile"
},
"resolutions": {
"@glimmer/manager": "0.84.3",
"@glimmer/interfaces": "0.84.3",
"@glimmer/runtime": "0.84.3",
"@glimmer/reference": "0.84.3",
"@glimmer/util": "0.84.3",
"@glimmer/validator": "0.84.3",
"@glimmer/manager": "0.88.0",
"@glimmer/interfaces": "0.88.0",
"@glimmer/runtime": "0.88.0",
"@glimmer/reference": "0.88.0",
"@glimmer/util": "0.88.0",
"@glimmer/validator": "0.88.0",
"@types/yargs": "17.0.13",
"@types/node": "^20.10.6",
"ember-cli-htmlbars": "^6.0.1"
Expand Down
12 changes: 6 additions & 6 deletions packages/core/__tests__/language-server/diagnostic-augmentation.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,11 +471,11 @@ describe('Language Server: Diagnostic Augmentation', () => {
"message": "The given value does not appear to be usable as a component, modifier or helper.",
"range": {
"end": {
"character": 26,
"character": 23,
"line": 14,
},
"start": {
"character": 4,
"character": 5,
"line": 14,
},
},
Expand Down Expand Up @@ -574,11 +574,11 @@ describe('Language Server: Diagnostic Augmentation', () => {
Property 'Foo' does not exist on type 'Globals'.",
"range": {
"end": {
"character": 7,
"character": 4,
"line": 1,
},
"start": {
"character": 0,
"character": 1,
"line": 1,
},
},
Expand All @@ -593,11 +593,11 @@ describe('Language Server: Diagnostic Augmentation', () => {
Property 'foo' does not exist on type 'Globals'.",
"range": {
"end": {
"character": 10,
"character": 7,
"line": 2,
},
"start": {
"character": 0,
"character": 1,
"line": 2,
},
},
Expand Down
168 changes: 102 additions & 66 deletions packages/core/__tests__/transform/debug.test.ts
View file
Open in desktop

Large diffs are not rendered by default.

22 changes: 15 additions & 7 deletions packages/core/__tests__/transform/template-to-typescript.test.ts
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

what's up with these changes? 😅

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'm not sure... i just run vite --update & prettier

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, wait, I did add a space in case its empty attributes

Original file line number Diff line number Diff line change
Expand Up @@ -758,7 +758,7 @@ describe('Transform: rewriteTemplate', () => {
const 𝛄 = χ.emitComponent(χ.resolve(Foo)());
χ.applyAttributes(𝛄.element, {
\\"data-bar\\": χ.resolve(helper)({ param: true , ...χ.NamedArgsMarker }),
});
});
}"
`);
});
Expand Down Expand Up @@ -792,7 +792,7 @@ describe('Transform: rewriteTemplate', () => {
const 𝛄 = χ.emitElement(\\"div\\");
χ.applyAttributes(𝛄.element, {
\\"data-attr\\": χ.resolveOrReturn(𝚪.args.input)(),
});
});
}"
`);
});
Expand All @@ -805,7 +805,7 @@ describe('Transform: rewriteTemplate', () => {
const 𝛄 = χ.emitElement(\\"div\\");
χ.applyAttributes(𝛄.element, {
\\"data-attr\\": \`\${χ.resolveOrReturn(𝚪.args.input)()}\`,
});
});
}"
`);
});
Expand Down Expand Up @@ -933,6 +933,8 @@ describe('Transform: rewriteTemplate', () => {
expect(templateBody(template, { globals: [] })).toMatchInlineSnapshot(`
"{
const 𝛄 = χ.emitElement(\\"div\\");
χ.applyAttributes(𝛄.element, {
});
χ.applyModifier(χ.resolve(modifier)(𝛄.element, { foo: \\"bar\\" , ...χ.NamedArgsMarker }));
}"
`);
Expand All @@ -944,6 +946,8 @@ describe('Transform: rewriteTemplate', () => {
expect(templateBody(template, { globals: [] })).toMatchInlineSnapshot(`
"{
const 𝛄 = χ.emitComponent(χ.resolve(MyComponent)());
χ.applyAttributes(𝛄.element, {
});
χ.applyModifier(χ.resolve(modifier)(𝛄.element, { foo: \\"bar\\" , ...χ.NamedArgsMarker }));
}"
`);
Expand All @@ -959,7 +963,7 @@ describe('Transform: rewriteTemplate', () => {
const 𝛄 = χ.emitElement(\\"div\\");
χ.applyAttributes(𝛄.element, {
\\"data-attr\\": χ.resolve(concat)(χ.resolve(foo)(1), χ.resolve(foo)(true)),
});
});
}"
`);
});
Expand Down Expand Up @@ -1060,7 +1064,7 @@ describe('Transform: rewriteTemplate', () => {
const 𝛄 = χ.emitElement(\\"div\\");
χ.applyAttributes(𝛄.element, {
\\"data-foo\\": χ.resolveOrReturn(𝚪.args.foo)(),
});
});
}"
`);
});
Expand All @@ -1073,7 +1077,7 @@ describe('Transform: rewriteTemplate', () => {
const 𝛄 = χ.emitElement(\\"div\\");
χ.applyAttributes(𝛄.element, {
\\"data-foo\\": \`\${χ.resolveOrReturn(𝚪.args.foo)()}\${χ.resolveOrReturn(𝚪.args.bar)()}\`,
});
});
}"
`);
});
Expand All @@ -1085,6 +1089,8 @@ describe('Transform: rewriteTemplate', () => {
"{
const 𝛄 = χ.emitElement(\\"div\\");
χ.applySplattributes(𝚪.element, 𝛄.element);
χ.applyAttributes(𝛄.element, {
});
}"
`);
});
Expand Down Expand Up @@ -1129,6 +1135,8 @@ describe('Transform: rewriteTemplate', () => {
"{
const 𝛄 = χ.emitComponent(χ.resolve(Foo)());
χ.applySplattributes(𝚪.element, 𝛄.element);
χ.applyAttributes(𝛄.element, {
});
}"
`);
});
Expand Down Expand Up @@ -1253,7 +1261,7 @@ describe('Transform: rewriteTemplate', () => {
const 𝛄 = χ.emitComponent(χ.resolve(NS?.Nested?.Custom)());
χ.applyAttributes(𝛄.element, {
class: \\"foo\\",
});
});
}
}
χ.Globals[\\"Foo\\"];
Expand Down
2 changes: 1 addition & 1 deletion packages/core/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
"typescript": ">=4.8.0"
},
"dependencies": {
"@glimmer/syntax": "^0.84.3",
"@glimmer/syntax": "^0.88.0",
"escape-string-regexp": "^4.0.0",
"semver": "^7.5.2",
"silent-error": "^1.1.1",
Expand Down
15 changes: 12 additions & 3 deletions packages/core/src/transform/diagnostics/augmentation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,10 @@ function checkResolveError(
// we need to traverse up through the path to find the true parent.
let sourceMapping = mapping.sourceNode.type === 'Identifier' ? mapping.parent : mapping;
let parentNode = sourceMapping?.parent?.sourceNode;
if (sourceMapping?.sourceNode.type === 'ElementNameNode') {
sourceMapping = sourceMapping.parent;
parentNode = sourceMapping?.parent?.sourceNode;
}

// If this error is on the first param to a {{component}} or other bind invocation, this means
// we either have a non-component value or a string that's missing from the registry.
Expand Down Expand Up @@ -176,7 +180,12 @@ function checkResolveError(
// Otherwise if this is on a top level invocation, we're trying to use a template-unaware
// value in a template-specific way.
let nodeType = sourceMapping?.sourceNode.type;
if (nodeType === 'ElementNode' || nodeType === 'PathExpression' || nodeType === 'Identifier') {
if (
nodeType === 'ElementNode' ||
nodeType === 'PathExpression' ||
nodeType === 'Identifier' ||
nodeType === 'ElementNameNode'
) {
return addGlintDetails(
diagnostic,
'The given value does not appear to be usable as a component, modifier or helper.'
Expand All @@ -201,8 +210,8 @@ function checkImplicitAnyError(

// This error may appear either on `<Foo />` or `{{foo}}`/`(foo)`
let globalName =
sourceNode.type === 'ElementNode'
? sourceNode.tag.split('.')[0]
sourceNode.type === 'ElementNameNode'
? sourceNode.value.split('.')[0]
: sourceNode.type === 'PathExpression' && sourceNode.head.type === 'VarHead'
Copy link

Choose a reason for hiding this comment

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

This kind of stuff should now work better, since ElementNode, MustachesStatement and SubExpression all have the same path property

? sourceNode.head.name
: null;
Expand Down
107 changes: 57 additions & 50 deletions packages/core/src/transform/template/template-to-typescript.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -595,39 +595,43 @@ export function templateToTypescript(
emit.indent();

emit.text('const 𝛄 = χ.emitComponent(χ.resolve(');
emitPathContents(path, start, kind);
emit.forNode(node.nameNode, () => {
emitPathContents(path, start, kind);
});
emit.text(')(');

let dataAttrs = node.attributes.filter(({ name }) => name.startsWith('@'));
if (dataAttrs.length) {
emit.text('{ ');

for (let attr of dataAttrs) {
emit.forNode(attr, () => {
start = template.indexOf(attr.name, start + 1);
emitHashKey(attr.name.slice(1), start + 1);
emit.text(': ');

switch (attr.value.type) {
case 'TextNode':
emit.text(JSON.stringify(attr.value.chars));
break;
case 'ConcatStatement':
emitConcatStatement(attr.value);
break;
case 'MustacheStatement':
emitMustacheStatement(attr.value, 'arg');
break;
default:
unreachable(attr.value);
}
});

start = rangeForNode(attr.value).end;
emit.text(', ');
}
emit.forNode(node.startTag, () => {
for (let attr of dataAttrs) {
emit.forNode(attr, () => {
start = template.indexOf(attr.name, start + 1);
emitHashKey(attr.name.slice(1), start + 1);
emit.text(': ');

switch (attr.value.type) {
case 'TextNode':
emit.text(JSON.stringify(attr.value.chars));
break;
case 'ConcatStatement':
emitConcatStatement(attr.value);
break;
case 'MustacheStatement':
emitMustacheStatement(attr.value, 'arg');
break;
default:
unreachable(attr.value);
}
});

start = rangeForNode(attr.value).end;
emit.text(', ');
}

emit.text('...χ.NamedArgsMarker }');
emit.text('...χ.NamedArgsMarker }');
});
}

emit.text('));');
Expand Down Expand Up @@ -759,7 +763,9 @@ export function templateToTypescript(
emit.indent();

emit.text('const 𝛄 = χ.emitElement(');
emit.text(JSON.stringify(node.tag));
emit.forNode(node.nameNode, () => {
emit.text(JSON.stringify(node.tag));
});
emit.text(');');
emit.newline();

Expand Down Expand Up @@ -793,35 +799,36 @@ export function templateToTypescript(
(attr) => !attr.name.startsWith('@') && attr.name !== SPLATTRIBUTES
);

if (!attributes.length) return;

emit.text('χ.applyAttributes(𝛄.element, {');
emit.newline();
emit.indent();
emit.forNode(node.startTag, () => {
emit.newline();
emit.indent();

let start = template.indexOf(node.tag, rangeForNode(node).start) + node.tag.length;
let start = template.indexOf(node.tag, rangeForNode(node).start) + node.tag.length;

for (let attr of attributes) {
emit.forNode(attr, () => {
start = template.indexOf(attr.name, start + 1);
for (let attr of attributes) {
emit.forNode(attr, () => {
start = template.indexOf(attr.name, start + 1);

emitHashKey(attr.name, start);
emit.text(': ');
emitHashKey(attr.name, start);
emit.text(': ');

if (attr.value.type === 'MustacheStatement') {
emitMustacheStatement(attr.value, 'attr');
} else if (attr.value.type === 'ConcatStatement') {
emitConcatStatement(attr.value);
} else {
emit.text(JSON.stringify(attr.value.chars));
}

emit.text(',');
emit.newline();
});
}
if (attr.value.type === 'MustacheStatement') {
emitMustacheStatement(attr.value, 'attr');
} else if (attr.value.type === 'ConcatStatement') {
emitConcatStatement(attr.value);
} else {
emit.text(JSON.stringify(attr.value.chars));
}

emit.dedent();
emit.text(',');
emit.newline();
});
}
// in case there are no attributes, this would allow completions to trigger
emit.text(' ');
emit.dedent();
});
emit.text('});');
emit.newline();
}
Expand Down
3 changes: 2 additions & 1 deletion packages/template/__tests__/emit-content.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { SafeString } from '@glimmer/runtime';
// @ts-expect-error cannot import, but we only need the type...
import type { SafeString } from '@glimmer/runtime';
import { htmlSafe } from '@ember/template';
Copy link

Choose a reason for hiding this comment

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

Why did it stop working? It seems to be here: https://github.com/glimmerjs/glimmer-vm/blob/v0.89.0/packages/%40glimmer/runtime/index.ts#L43

And I don't think we would want @ts-expect-error here, if the only reason we want to import it is for the type, but specifically the type import wasn't working, what does this even do? (making it any, probably)

Copy link
Contributor Author

@patricklx patricklx Mar 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This was the error:
https://github.com/typed-ember/glint/actions/runs/7712187814/job/21019163076#step:9:139

tests__/emit-content.test.ts(1,28): error TS1479: The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider writing a dynamic 'import("@glimmer/runtime")' call instead.

import { emitContent } from '../-private/dsl';

Expand Down
1 change: 1 addition & 0 deletions tsconfig.compileroptions.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
"composite": true,
"declaration": true,
"sourceMap": true,
"skipLibCheck": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

this may be controversial with the former ember-TS members, but I don't know of another way to have a node16 project interoperable with non-none16 projects

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to fix this issue upstream in the glimmer side. skipLibCheck is a pretty blunt tool that can hide problems globally (since type failures in a library can generate anys that then propagate through our own code).

Copy link
Contributor

Choose a reason for hiding this comment

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

glimmerjs/glimmer-vm#1555

"types": []
}
}
Loading
Loading