typed-ember / glint

TypeScript powered tooling for Glimmer templates
https://typed-ember.gitbook.io/glint
MIT License
108 stars 50 forks source link

Make VS Code plugin work for pnpm projects #524

Closed simonihmig closed 5 months ago

simonihmig commented 1 year ago

I struggled to get the VS Code plugin to work in a monorepo using pnpm (if you want to reproduce: https://github.com/CrowdStrike/ember-headless-form). In the Output tab I would see:

Unable to resolve @glint/core from /Users/sihmig/Projects/oss/ember-headless-form — not launching Glint for this directory.

Which makes sense, as pnpm does not hoist the glint dependencies into the root node_modules as yarn or npm would do.

I finally got it to work by

Leaving this here for visibility...

But also wondering if the plugin could be smarter about this, like trying to resolve glint from each workspace that has an Glint-enabled tsconfig.json instead of the repo's root?

dfreeman commented 1 year ago

But also wondering if the plugin could be smarter about this, like trying to resolve glint from each workspace that has an Glint-enabled tsconfig.json instead of the repo's root?

We're not going to have the extension crawl your workspace to try and discover locations where @glint/core might be — aside from the impact that would have on your editor startup time, it's also not clear what we should do if we discover more than one copy.

upgrading the plugin to the pre-release version (v1.0.1) instead of latest stable v0.9.1, by clicking "Switch to Pre-Release Version"

I think when we release the next 1.0 beta, we'll probably promote the VS Code extension to a stable release since it's largely backwards compatible with older versions of @glint/core anyway. When we do that, we'll also update the docs to capture this more clearly, and it may make sense to update that message in the Output tab to mention the config option as well.

Note that this is consistent with how Code's own editor tooling for TypeScript behaves—it will only load a workspace-local copy of TS if it can resolve it from the workspace root or if you explicitly configure it to look in another location.

The key difference is that Code also bundles a copy of TypeScript itself that it will use if you don't have it set up to locate one in your workspace. We may explore doing something similar down the line as Glint stabilizes, but even though TypeScript is much more stable and mature and has fairly infrequent major changes, there's still often user confusion when their editor's version of tsserver gives different diagnostics than what they see when they run tsc at the command line.

simonihmig commented 1 year ago

Thanks for the feedback!

Would it make sense to recommend to monorepo users (apps or v2 addons) to install @glint/core in the workspace root? (as in yarn add -W @glint/core)

This would allow it for any dev working on the monorepo to "just work", and not have to tweak VSCode settings (when not committing those in to the repo). Also we could bake that into the v2 blueprint to work ootb. /cc @NullVoxPopuli

NullVoxPopuli commented 1 year ago

No, we don't want to add fake coventions to do what bad package managers do.

Instead, i think the glint extension should use find-up when a file is opened to find the nearest package.json/tsconfig.json and use that to check if @glint/core is defined s, then require resolve from there.

Actually, if the glint extension just used require resolve from the directory of the open file, that would solve everything. No need for dependency location assumptions (which no project should do, esp as people keep making new package managers.. by ening require.resolve, we don't need to support any package manager, because we 'do the right thing')

This line: https://github.com/typed-ember/glint/blob/main/packages/vscode/src/extension.ts#L162

Needs the second argument to scope the require to the active file/workspace: https://nodejs.org/api/modules.html#requireresolverequest-options

dfreeman commented 1 year ago

Actually, if the glint extension just used require resolve from the directory of the open file, that would solve everything.

If you look directly above that line, you'll see that we're already "just" being quite intentional about where we're resolving @glint/core from 😉

However, we don't want to launch a separate language server process for each tsconfig.json in your workspace. Again, this is consistent with how Code's own TypeScript tooling works — a single tsserver process is shared across your active projects within a given workspace root, which both cuts down on process overhead and allows for reuse of parsing and typechecking information for common files like TS's baked-in library declarations.

simonihmig commented 1 year ago

We're not going to have the extension crawl your workspace to try and discover locations where @glint/core might be

Would that really be such an expensive operation, to find the first Glint-enabled tsconfig.json file (excluding **/node_modues) and use that as the resolutionDir (in case glint.libraryPath is not set), only once when the plugin initializes?

dfreeman commented 1 year ago

find the first Glint-enabled tsconfig.json file (excluding **/node_modues) and use that as the resolutionDir

Doing that would imply that either a) we're spinning up separate language server instances per Glint-enabled tsconfig.json, or b) the order you happen to open files in the editor in will determine which version of the language server is booted. Neither of those is a situation we want to be in.

Realistically, we're not going to make a different design decision than TypeScript on something like this. Even if I didn't agree with their choices (though in this case I do!), keeping our behavior consistent with what the "native" version of the tooling does is going to be simpler for users to build a mental model for and easier for us to maintain in the face of future evolution. To that end, I think the only change we'd be likely to make on this, other than documentation/error message improvements, would be potentially bundling a copy of @glint/core within the Code extension.

As I said above, that's something we may ultimately do—it has some significant advantages! But it also comes with meaningful tradeoffs and further design considerations we'd need to think through 🙂

NullVoxPopuli commented 8 months ago

Should this be closed since there are two paths forward?

NullVoxPopuli commented 5 months ago

closing, as I (and others!) have been using Glint in pnpm projects for quite some time now.