use new glimmer ElementNode sub nodes #696

patricklx · 2024-01-29T14:45:40Z

there are now also following nodes:

name node: ElementNameNode
start tag node: ElementStartNode

example:

/*
  <Foo.bar.x attr='2'></Foo.bar.x>
   ^-- ElementPartNode
       ^-- ElementPartNode
          ^- ElementPartNode
   ^-------- ElementNameNode
  ^------------------ ElementStartNode
                      ^----------- ElementEndNode
 */

afterwards completions/validations can be add for attributes and element tags
In #663

NullVoxPopuli · 2024-01-29T15:11:52Z

test-packages/ts-ember-app/app/components/foo.hbs

there are a lot of noisy changes here -- can you reduce the diff to what was meaningful? thanks!

patricklx · 2024-01-30T10:43:12Z

@NullVoxPopuli any idea how I can fix this:

Error: node_modules/@glimmer/interfaces/index.d.ts(1,2[9](https://github.com/typed-ember/glint/actions/runs/7698348329/job/20977417907?pr=696#step:5:10)): error TS2834: Relative import paths need explicit file extensions in EcmaScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Consider adding an extension to the import path.
Error: node_modules/@glimmer/interfaces/index.d.ts(3,15): error TS2834: Relative import paths need explicit file extensions in EcmaScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Consider adding an extension to the import path.
Error: node_modules/@glimmer/interfaces/index.d.ts(4,15): error TS2834: Relative import paths need explicit file extensions in EcmaScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Consider adding an extension to the import path.
Error: node_modules/@glimmer/interfaces/index.d.ts(5,15): error TS2834: Relative import paths need explicit file extensions in EcmaScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Consider adding an extension to the import path.
Error: node_modules/@glimmer/interfaces/index.d.ts(6,15): error TS2834: Relative import paths need explicit file extensions in EcmaScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Consider adding an extension to the import path.
Error: node_modules/@glimmer/interfaces/index.d.ts(7,15): error TS2834: Relative import paths need explicit file extensions in EcmaScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Consider adding an extension to the import path.

NullVoxPopuli · 2024-01-30T12:57:00Z

Does skipLibCheck help?

Otherwise, glimmer has more bugs we need to fix (all relative imports should always use extensions when using node16, so it shouldn't be too bad of a fix)

patricklx · 2024-01-30T13:00:39Z

Does skipLibCheck help?

Otherwise, glimmer has more bugs we need to fix (all relative imports should always use extensions when using node16, so it shouldn't be too bad of a fix)

yes, it helped!

NullVoxPopuli · 2024-01-30T15:00:16Z

packages/core/__tests__/transform/template-to-typescript.test.ts

what's up with these changes? 😅

i'm not sure... i just run vite --update & prettier

ah, wait, I did add a space in case its empty attributes

NullVoxPopuli · 2024-01-30T15:16:44Z

tsconfig.compileroptions.json

@@ -11,6 +11,7 @@
    "composite": true,
    "declaration": true,
    "sourceMap": true,
+    "skipLibCheck": true,


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

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).

glimmerjs/glimmer-vm#1555

NullVoxPopuli · 2024-01-30T15:18:09Z

@patricklx would there be any noticeable editor behavior changes with this PR?

(or is it "just the same, but using the new parser capabilities"?)

patricklx · 2024-01-30T15:22:51Z

i'm not seeing and difference

NullVoxPopuli

Let's block on glimmerjs/glimmer-vm#1555
And get that update in here, too once released

NullVoxPopuli · 2024-02-01T00:34:59Z

@patricklx new versions published, can you try the .1 release?

patricklx · 2024-02-01T10:58:03Z

nope, some new errors-...

Error: node_modules/@types/ember__helper/index.d.ts(3,81): error TS147[9](https://github.com/typed-ember/glint/actions/runs/7739973176/job/21103924609?pr=696#step:5:10): 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/manager")' call instead.
Error: node_modules/@types/ember__helper/index.d.ts(4,53): 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.
Error: node_modules/@glimmer/modifier/dist/commonjs/index.d.ts(1,20): 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.

microsoft/TypeScript#52529
microsoft/TypeScript#53426

looks like @ef4 knows something about this

NullVoxPopuli · 2024-02-01T15:00:40Z

ah yeah, we're probably blocked on ember-source needing to actually ship as a type=module package.

patricklx · 2024-02-01T16:53:04Z

@NullVoxPopuli the error is under @types folder.
So we can make that type: module?
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/ember__helper/package.json

And here?
https://github.com/patricklx/glimmer.js/blob/master/packages/%40glimmer/modifier/package.json

NullVoxPopuli · 2024-02-01T22:10:08Z

does changing all of @types/ember* to type=module work locally? 🤔

patricklx · 2024-02-02T08:51:09Z

does changing all of @types/ember* to type=module work locally? 🤔

it fixes some errors,
but also needed to fix @glimmerx/modifier and @glimmer/modifier.

still, at the end it says
node_modules/@types/ember__helper/index.d.ts:1:24 - error TS2307: Cannot find module 'ember/-private/type-utils' or its corresponding type declarations.

NullVoxPopuli · 2024-02-02T12:57:19Z

Ahh, yeah, probably because all the private relative imports need extensions at that point. Hmm

NullVoxPopuli · 2024-02-02T17:51:16Z

I've added skipLibCheck back.

It doesn't seem we can build without it.

… all the @types to ESM, which would break ember apps, etc

chancancode · 2024-03-11T18:48:49Z

@patricklx FYI 0.89.0 is out with glimmerjs/glimmer-vm#1568 The information you need should now be in ElementNode.{path,openTag,closeTag}: glimmerjs/glimmer-vm@68509ac#diff-21f1f3fd9aaae780472e1cf9cf079d1a05efd0fd642f6758c963dc7e969a4b30

Should also fix #706

chancancode · 2024-03-11T18:56:48Z

packages/core/src/transform/diagnostics/augmentation.ts

-      sourceNode.type === 'ElementNode'
-        ? sourceNode.tag.split('.')[0]
+      sourceNode.type === 'ElementNameNode'
+        ? sourceNode.value.split('.')[0]
        : sourceNode.type === 'PathExpression' && sourceNode.head.type === 'VarHead'


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

chancancode · 2024-03-11T18:59:42Z

packages/template/__tests__/emit-content.test.ts

@@ -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';


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)

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.

patricklx added 2 commits January 29, 2024 15:45

update glimmer

eb16ae6

fix

eb75445

patricklx force-pushed the update-glimmer branch from 4e5ee8b to d7f88cb Compare January 29, 2024 15:09

NullVoxPopuli reviewed Jan 29, 2024

View reviewed changes

patricklx force-pushed the update-glimmer branch 2 times, most recently from 4a810e2 to 23ef8a2 Compare January 29, 2024 15:15

for component too

f0bf4c4

patricklx force-pushed the update-glimmer branch from 23ef8a2 to f0bf4c4 Compare January 29, 2024 15:20

patricklx added 6 commits January 30, 2024 14:01

fix build

240b8d4

fix lint

d9f6d21

fix tests

3be0379

fix lint

962ec06

ignore

a6f4f07

ignore

ef094ec

patricklx requested a review from NullVoxPopuli January 30, 2024 14:45

NullVoxPopuli reviewed Jan 30, 2024

View reviewed changes

fix

15286b9

NullVoxPopuli reviewed Jan 30, 2024

View reviewed changes

NullVoxPopuli approved these changes Jan 30, 2024

View reviewed changes

NullVoxPopuli mentioned this pull request Jan 30, 2024

@glimmer/interfaces now uses extensions in its imports. glimmerjs/glimmer-vm#1555

Merged

NullVoxPopuli requested changes Jan 30, 2024

View reviewed changes

revert

bf1df59

fix

7103b7d

NullVoxPopuli mentioned this pull request Feb 2, 2024

Cannot find module '@glimmer/env' when trying to run glint --declaration #698

Open

We *have* to skipLibCheck, or else we can't build. We have to convert…

3fedc71

… all the @types to ESM, which would break ember apps, etc

chancancode reviewed Mar 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use new glimmer ElementNode sub nodes #696

use new glimmer ElementNode sub nodes #696

patricklx commented Jan 29, 2024 •

edited

NullVoxPopuli Jan 29, 2024

patricklx commented Jan 30, 2024

NullVoxPopuli commented Jan 30, 2024 •

edited

patricklx commented Jan 30, 2024

NullVoxPopuli Jan 30, 2024

patricklx Jan 30, 2024

patricklx Jan 30, 2024

NullVoxPopuli Jan 30, 2024

ef4 Jan 30, 2024

NullVoxPopuli Jan 30, 2024

NullVoxPopuli commented Jan 30, 2024

patricklx commented Jan 30, 2024

NullVoxPopuli left a comment

NullVoxPopuli commented Feb 1, 2024

patricklx commented Feb 1, 2024

NullVoxPopuli commented Feb 1, 2024

patricklx commented Feb 1, 2024 •

edited

NullVoxPopuli commented Feb 1, 2024

patricklx commented Feb 2, 2024

NullVoxPopuli commented Feb 2, 2024

NullVoxPopuli commented Feb 2, 2024 •

edited

chancancode commented Mar 11, 2024 •

edited

chancancode Mar 11, 2024

chancancode Mar 11, 2024

patricklx Mar 11, 2024 •

edited

use new glimmer ElementNode sub nodes #696

Are you sure you want to change the base?

use new glimmer ElementNode sub nodes #696

Conversation

patricklx commented Jan 29, 2024 • edited

Choose a reason for hiding this comment

patricklx commented Jan 30, 2024

NullVoxPopuli commented Jan 30, 2024 • edited

patricklx commented Jan 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli commented Jan 30, 2024

patricklx commented Jan 30, 2024

NullVoxPopuli left a comment

Choose a reason for hiding this comment

NullVoxPopuli commented Feb 1, 2024

patricklx commented Feb 1, 2024

NullVoxPopuli commented Feb 1, 2024

patricklx commented Feb 1, 2024 • edited

NullVoxPopuli commented Feb 1, 2024

patricklx commented Feb 2, 2024

NullVoxPopuli commented Feb 2, 2024

NullVoxPopuli commented Feb 2, 2024 • edited

chancancode commented Mar 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patricklx Mar 11, 2024 • edited

Choose a reason for hiding this comment

patricklx commented Jan 29, 2024 •

edited

NullVoxPopuli commented Jan 30, 2024 •

edited

patricklx commented Feb 1, 2024 •

edited

NullVoxPopuli commented Feb 2, 2024 •

edited

chancancode commented Mar 11, 2024 •

edited

patricklx Mar 11, 2024 •

edited