Skip to content

Refrain creating ember-loose virtual code in non ember-loose environment#851

Merged
NullVoxPopuli merged 1 commit intotyped-ember:mainfrom
mogstad:push-psqwnznyzxkn
Mar 26, 2025
Merged

Refrain creating ember-loose virtual code in non ember-loose environment#851
NullVoxPopuli merged 1 commit intotyped-ember:mainfrom
mogstad:push-psqwnznyzxkn

Conversation

@mogstad
Copy link
Copy Markdown
Contributor

@mogstad mogstad commented Mar 26, 2025

I didn’t find a great place to test this, so ended up adding to the vscode test suite. Any better suggestions are very welcome.
Closes #846

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

lgtm

@NullVoxPopuli NullVoxPopuli added the bug Something isn't working label Mar 26, 2025
@NullVoxPopuli NullVoxPopuli merged commit e599e31 into typed-ember:main Mar 26, 2025
2 checks passed
@github-actions github-actions Bot mentioned this pull request Mar 26, 2025
]);
});

test.only('gives diagnostics for TypeScript file', async () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This .only() needs to be removed so that other tests run. Normally vitest catches this on CI but these VSCode tests I think use mocha or something that doesn't catch cases like these.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bah. I missed this :(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fix here #852

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah sorry!

@machty
Copy link
Copy Markdown
Contributor

machty commented Mar 26, 2025

Nice, I can see some perf gains here; I wanted to guard for this originally but with some of the trickiness in TS Plugin's synchronous API it wasn't clear that we'd be able to synchronously determine the presence of ember-loose. Glad you were able to bring it back.

@machty
Copy link
Copy Markdown
Contributor

machty commented Mar 26, 2025

FWIW I think there might be further potential perf gains here, because even with your PR, apps with ember-loose are still going to be parsing EVERY .ts file into the Loose VirtualCode upon the possibility that it contains a classic loose mode Component definition, i.e. even a file containing an Ember Data / WarpDrive model is going to be parsed into VirtualCode.

Part of the reason I did it this way was 1. just to get something working and 2. based on angular-webstorm's experience. With angular (pardon me bc I could be getting something wrong), a .ts file is considered a component once the @Component decorator is introduced, which might seem like an obvious / easy thing to detect to determine whether to parse into VirtualCode, but I think they opted to just parse ALL .ts into VirtualCode because, unless you do, with the way Volar works, Volar wouldn't be able to detect that a .ts file that previously didn't have a @Component should suddenly then be parsed into VirtualCode once @Component is added.

That said angular-webstorm might have additional perf checks to do a light-weight parse within the VirtualCode if no @Component is detected.

In our case, perhaps it's feasible to check whether a .ts file has /components/ in its path? Are there any cases of classic loose mode components being defined in, say, a src/ directory but not specifically in a components/ directory?

@NullVoxPopuli
Copy link
Copy Markdown
Contributor

In v2 addons they can be anywhere. But default paths are /components/

Its worth the perf. Maybe we can add an option to add more component paths in TSconfig if needed

@machty
Copy link
Copy Markdown
Contributor

machty commented Mar 26, 2025

Summarizing some of our discussions in DM:

The only potential downside/uncovered case by this perf improvement would be that we can't support the use case of v2 addons that still use loose mode components AND their components don't have a components/ folder in their folder ancestry. In such a case the addon authors wouldn't see type-checking diagnostics in their .hbs files. This is pretty tiny corner case seeing as how most folk on v2 have probably converted to .gts, and if not it's easy to have them move their components into components/ (or keep them there if they're already there) if they want the Glint 2 experience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

V2: JS/TS language service crashes if only ember-template-imports is specified in tsconfig glint.environment

3 participants