fastify-autoload icon indicating copy to clipboard operation
fastify-autoload copied to clipboard

feat: add typescript support for `vite` and `vite-node`

Open xeho91 opened this issue 1 year ago • 13 comments

Resolves #226

Checklist

xeho91 avatar Dec 17 '23 07:12 xeho91

NOTES:

  • I encountered the same issue #226, and I thought there could be a way to work around it. I personally used vite-node, but same case could happen with using vite too.
  • The solution provided is simply checking if the basename of the process.argv[1] returns a bin filename for both vite and vite-node. Stripping extension - I've left comment with link to the respective package.json files, just in case they would change the extension name. Who knows?
  • I am not sure if this ideal solution.
  • There was no npm run benchmark script available, I guess this PR template is general across your repositories, so I assumed npm run test was enough.
  • I've got no idea how to write a test/or benchmark for this case. If they're needed I would appreciate the guidance on how to write one.
  • I love Fastify ❤️

xeho91 avatar Dec 17 '23 07:12 xeho91

Woops, I might have broken something!

As the tests are failing for the Node.js v18, however they pass for the latest version (v21).

I'm not knowledgeable enough to understand if this test is strictly related to the changes I implemented. As far I understood the code, and the fact that I'm only using the basename from standard library - node:path. I don't see the connection with the error message, hence why I would appreciate any insight or guidance.

xeho91 avatar Dec 18 '23 10:12 xeho91

I was able to reproduce the issue locally.

The fix is to upgrade tsx to latest version - see #344. So, this PR depends on it in order for the test to pass.

xeho91 avatar Dec 18 '23 11:12 xeho91

I'm ok in dropping Node v14 and v16 with this PR, but it should still support Node v18. Right now I do not have much time to dig into why this wasn't working for Vite, every time I did a solution eluded me. Can you add a test too if manage to make current one passing?

mcollina avatar Dec 18 '23 15:12 mcollina

I'm ok in dropping Node v14 and v16 with this PR, but it should still support Node v18. Right now I do not have much time to dig into why this wasn't working for Vite, every time I did a solution eluded me. Can you add a test too if manage to make current one passing?

  1. Oh I see, upgrading tsx also comes with a cost of latest supported version of Node.js will be from v18.
  2. Testing. Sure, I can provide tests. However, at the current moment I don't have a idea for a best test approach for it.

Once I'll have a free time, I'll sit on this issue again and will think about how to write a test for this one. If you have any hints, I'll be grateful!

[!IMPORTANT] Important note as well, in case I forget again in the future. If using with vite, then we need to provide a loader:

NODE_OPTIONS="--loader=tsx" vite

xeho91 avatar Dec 27 '23 11:12 xeho91

I'm ok to ship a new major and support v18.19+.

mcollina avatar Dec 27 '23 11:12 mcollina

See e.g.https://github.com/fastify/fastify-autoload/pull/280/files for implementing a test

But if you can provide an example repo where you use vite and node-vite and your fork to load your vite and node-vite code, I could add the test to this PR.

Uzlopak avatar Dec 27 '23 11:12 Uzlopak

Update

Firstly, thank you for you patience, as the Lunar New Year is almost here, I finally have time to finish unresolved matters.

✅ So, I've managed to add test for when running with vite-node.

❌ However, I'm unable to configure tests properly for building via vite. I've had errors related to not being able to use Node.js builtin modules in the browser such as this one:

RollupError: "dirname" is not exported by "__vite-browser-external"

I've tried to do a lot of manipulation via vite.config.ts, and eventually I lost track of time trying to reproduce on how I was able to make it work during my experimenting a while ago.


So, if the test is really mandatory for using with vite (I'm using vite-node instead)... I propose that I could just drop (remove) this extra attempt to support using vite, as I failed to provide a test for this case, but we could move forward with using vite-node.

xeho91 avatar Feb 07 '24 09:02 xeho91

Well, the goal was to support vite, not vite-node. I'm okay with you amending the PR only to support vite-node.

What's the vite.config.ts that you are trying to use? It seems that you are trying to bundle fastify-autoload for a browser environment, which is unlikely to work, given that it needs access to the file system.

mcollina avatar Feb 07 '24 14:02 mcollina

Well, the goal was to support vite, not vite-node. I'm okay with you amending the PR only to support vite-node.

What's the vite.config.ts that you are trying to use? It seems that you are trying to bundle fastify-autoload for a browser environment, which is unlikely to work, given that it needs access to the file system.

Okay, I didn't want to give up on this so easily after all.

I know the error message seems like as I was trying to build for browser, while I used the lib option and target: ["node20"]. I was clueless at this point 🤷 as to what is going on.

So, today with fresh mind, I attempted again to write tests for vite, and I also found on my device the experimental repository from a few months ago where I was able to bundle fastify app with vite only.

The missing link was that I was not using the vite-plugin-node. Using this plugin almost solved this problem, I encountered this error:

RollupError: "default" is not exported by "index.js", imported by "test/typescript/basic/app.ts".

So, I knew I had to modify the rollupOptions. And it was enough just to add another plugin @rollup/plugin-commonjs.

And voila, it worked ✅.

So, to summarize: In the provided tests for vite, I'm testing building for:

  • /test/typescript/
  • /test/typescript-esm/

In the end, I think using vite for bundling an Node.js app seems to be a bit too much of a hassle after all - on the setup part. Using vite-node for bundling the fastify app is just more simpler.

xeho91 avatar Feb 08 '24 12:02 xeho91

CI is failing

mcollina avatar Feb 08 '24 12:02 mcollina

I need help with this one. By default I am using pnpm as package manager.

So, I reinstalled the project dependencies by using npm via Node.js LTS (v20). I reproduced the same issue as on the CI.

What helped was adding the flag --legacy-peer-deps.

npm i --ignore-scripts --legacy-peer-deps

Not sure how else I can fix it, and I'd rather to not modify the fastify/workflows repo (the organisation shared ones) without your permission.

xeho91 avatar Feb 08 '24 12:02 xeho91

Apparently vite-plugin-node requires vite v4.0.0: https://github.com/axe-me/vite-plugin-node/blob/b9ff47f6f02b4ffd4648812a3de6d5d133879ac6/packages/vite-plugin-node/package.json#L41

I think you can set legacy-peeer-deps in the .npmrc: https://github.com/npm/rfcs/discussions/283

mcollina avatar Feb 08 '24 13:02 mcollina

Closing due to inactivity.

climba03003 avatar Jul 16 '24 08:07 climba03003