analog
analog copied to clipboard
feat(vite-plugin-nitro): add support for typescript paths import resolution
PR Checklist
Please check if your PR fulfills the following requirements:
- [X] The commit message follows our guidelines: https://github.com/analogjs/analog/blob/main/CONTRIBUTING.md#-commit-message-guidelines
- [] Tests for the changes have been added (for bug fixes / features)
- [] Docs have been added / updated (for bug fixes / features)
PR Type
What kind of change does this PR introduce?
- [ ] Bugfix
- [X] Feature
- [ ] Code style update (formatting, local variables)
- [ ] Refactoring (no functional changes, no api changes)
- [ ] Build related changes
- [ ] CI related changes
- [ ] Documentation content changes
- [ ] Other... Please describe:
Which package are you modifying?
- [ ] vite-plugin-angular
- [X] vite-plugin-nitro
- [ ] astro-angular
- [ ] create-analog
- [ ] router
- [X] platform
- [ ] content
- [X] nx-plugin
- [ ] trpc
What is the current behavior?
Currently typescript imports using the compilerOptions.paths aliases set in tsconfig.json are not automatically handled for Nitro. This causes the nitro server to fail to run when executing the imports. This is especially evident in a Nx monorepo leveraging multiple in-built libraries.
Closes # Discord Thread
What is the new behavior?
This PR adds rollup-plugin-typescript-paths as a dependency of vite-plugin-nitro. This plugin is included as one of the plugins within the rollupConfig of the NitroConfig.
The plugin is configured to default to loading / reading from a tsconfig.json file in the root of the repo. However, to support other setups an option was added to both the Options for vite-plugin-nitro and the @analog/platform plugin to allow a user to override the tsconfig file / path.
Related to the 2nd point, the nx-plugin was update to specify tsconfig.base.json as the tsconfig file to load. This was added as a default for all current vite.config.ts templates in the plugin.
Does this PR introduce a breaking change?
- [ ] Yes
- [X] No
Other information
[optional] What gif best describes this PR or how it makes you feel?
Deploy Preview for analog-docs ready!
| Name | Link |
|---|---|
| Latest commit | 6edaeba403c6370891daf807d6a950ceb6d7d83f |
| Latest deploy log | https://app.netlify.com/sites/analog-docs/deploys/6577a7106c6a210008136a9d |
| Deploy Preview | https://deploy-preview-792--analog-docs.netlify.app |
| Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site configuration.
Deploy Preview for analog-blog ready!
| Name | Link |
|---|---|
| Latest commit | 6edaeba403c6370891daf807d6a950ceb6d7d83f |
| Latest deploy log | https://app.netlify.com/sites/analog-blog/deploys/6577a7105793ce0008f0eaf9 |
| Deploy Preview | https://deploy-preview-792--analog-blog.netlify.app |
| Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site configuration.
Deploy Preview for analog-app ready!
| Name | Link |
|---|---|
| Latest commit | 6edaeba403c6370891daf807d6a950ceb6d7d83f |
| Latest deploy log | https://app.netlify.com/sites/analog-app/deploys/6577a710fe3ce200087eb023 |
| Deploy Preview | https://deploy-preview-792--analog-app.netlify.app |
| Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site configuration.
@brandonroberts wanted to follow up on this PR. Anything further I should look to address?
@jeremyhofer looks good in general. Just a little stuck on the naming because we already have a tsconfig property the vite object, which is different from this one and may cause some confusion.
analog({
vite: { tsconfig: 'path/to/tsconfig.json' },
tsConfigPath: 'tsconfig.base.json'
})
Maybe it should be workspaceTsConfig to go along with workspaceRoot.
Thoughts?
@brandonroberts been experimenting with this and thinking more about how to best handle this, and want your thoughts.
First, some additional things I've determined regarding tsconfig paths support in analog today:
- The import issue the PR is currently targeting only impacts running the Nitro server (
run,serve,buildand execute, and similar targets). It specifically does not have an impact on thetesttarget. - Using tsconfig path imports within components / pages and unit tests cannot resolve imports either. However, this is resolved via
vite-tsconfig-pathsinstead of the rollup/nitro plugin. Nx has migrations that checks for and addsvite-tsconfig-pathsto yourvite-config.tsfile if it is not present, so I was not seeing the issue in my repo. However, removing that plugin caused the paths to not be resolved. Similarly, in a new standlone analog app, if you configure paths intsconfig.jsonthey are not recognized.
Regarding the current approach in this PR targeting a fix for the Nitro server - I think we can get by without configuring a tsconfig file, instead just referencing tsconfig.app.json. This ultimately extends the root tsconfig.json in a standalone analog project, or the tsconfig.base.json in a Nx repo. Unfortunately the rollup-plugin-typescript-paths dependency I added does not use typescript to fully resolve the tsconfig, so extends inheritance is not respected. This will have to be manually tooled.
I'm not having luck finding a rollup plugin that handles the tsconfig paths in a way that works with analog. Rollup provides the @rollup/plugin-typescript, but it requires specific tsconfig configurations, especially related to the outDir, that do not work with how analog bundles the application. Other plugins similarly either are doing too much or not enough to fit the analog usecase.
Given all the above, I'm currently considering the following:
- Manually implementing a rollup plugin to handle the tsconfig paths. Functionally it be like
rollup-plugin-typescript-paths, but will use the typescript compiler to resolve the tsconfig and handle all inheritance, as the@rollup/plugin-typescriptplugin does. This plugin will referencetsconfig.app.json. - Adding
vite-tsconfig-pathsas a dependency of@analogjs/platformand including the plugin in the set of plugins provided via the platform plugin, as this is needed for all project targets.
Thoughts or concerns on any of the above?
Option 2 is a better solution from a long-term maintenance standpoint. Nx uses their own tsconfig paths plugin, will this cause any conflicts with that?
In this case what I recommended weren't different options, I'd have to do both items 1 and 2 from the bottom of my last comment to resolve tsconfig path support for all use cases.
For item 1 I called out the question is around if we're ok implementing the rollup plugin to support tsconfig paths for nitro / rollup, which is needed for bundling / running the server. Alternative would be to work the @rollup/plugin-typescript plugin into vite-plugin-nitro, but this may drive refactoring in vite-plugin-nitro.
For item 2, good callout on Nx's tsconfig path plugin. I just saw that this evening while experimenting with my test related posts. In my current project Nx had added the vite-tsconfig-paths plugin and configured it in a migration, their own plugin wasn't introduced yet. I'll experiment with this and see if there's any issues with analog including a vite tsconfig path plugin by default.
I've seen a couple others run into this also, but I think we would be better off adding this as a section in the Nx guide where its most likely to happen.
https://analogjs.org/docs/integrations/nx
Thoughts?