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 new glimmer ElementNode sub nodes #696

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

Conversation

patricklx
Copy link
Contributor

@patricklx patricklx commented Jan 29, 2024

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

@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
Copy link
Contributor

NullVoxPopuli commented Jan 30, 2024

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
Copy link
Contributor Author

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!

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

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

@NullVoxPopuli
Copy link
Contributor

@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
Copy link
Contributor Author

i'm not seeing and difference

Copy link
Contributor

@NullVoxPopuli NullVoxPopuli left a comment

Choose a reason for hiding this comment

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

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

@NullVoxPopuli
Copy link
Contributor

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

@patricklx
Copy link
Contributor Author

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
Copy link
Contributor

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

@patricklx
Copy link
Contributor Author

patricklx commented Feb 1, 2024

@NullVoxPopuli
Copy link
Contributor

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

@patricklx
Copy link
Contributor Author

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
Copy link
Contributor

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

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Feb 2, 2024

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
Copy link

chancancode commented Mar 11, 2024

@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

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

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

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

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

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.

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