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

feat: add typescript support for vite and vite-node #346

Closed
wants to merge 6 commits into from

Conversation

xeho91
Copy link

@xeho91 xeho91 commented Dec 17, 2023

Resolves #226

Checklist

@xeho91
Copy link
Author

xeho91 commented Dec 17, 2023

NOTES:

  • I encountered the same issue fastify-autoload cannot import plugin...To fix this error compile TypeScript to JavaScript or use 'ts-node' to run your app. #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
Copy link
Author

xeho91 commented Dec 18, 2023

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

xeho91 commented Dec 18, 2023

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.

@mcollina
Copy link
Member

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?

@xeho91
Copy link
Author

xeho91 commented Dec 27, 2023

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

@mcollina
Copy link
Member

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

@Uzlopak
Copy link
Contributor

Uzlopak commented Dec 27, 2023

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.

@xeho91
Copy link
Author

xeho91 commented Feb 7, 2024

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.

@mcollina
Copy link
Member

mcollina commented Feb 7, 2024

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.

@xeho91
Copy link
Author

xeho91 commented Feb 8, 2024

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.

@mcollina
Copy link
Member

mcollina commented Feb 8, 2024

CI is failing

@xeho91
Copy link
Author

xeho91 commented Feb 8, 2024

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.

@mcollina
Copy link
Member

mcollina commented Feb 8, 2024

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: npm/rfcs#283

@climba03003
Copy link
Member

Closing due to inactivity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants