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 NodeJS version #23

Merged
merged 11 commits into from
Dec 28, 2024
Merged

feat: add NodeJS version #23

merged 11 commits into from
Dec 28, 2024

Conversation

rtritto
Copy link
Contributor

@rtritto rtritto commented Sep 28, 2024

Create a NodeJS version, so it can be used with https://github.com/elysiajs/node-adapter

Differences

  • use fs.globSync (it's introduced with NodeJS 22.0.0) instead of Bun.glob
  • use fs.writeFileSync instead Bun.write

Notes:

TODO

  • test build
  • .ts/.tsx files needs to be transpiled

Fix #30

@rtritto rtritto marked this pull request as draft September 28, 2024 22:14
@kravetsone
Copy link
Owner

Why not typeof bun === "undefined"

@kravetsone
Copy link
Owner

Also we should check that esbuild plugin not broken with it

@rtritto rtritto marked this pull request as ready for review September 29, 2024 04:26
@rtritto
Copy link
Contributor Author

rtritto commented Sep 29, 2024

Why not typeof bun === "undefined"

Done, thanks!

@rtritto
Copy link
Contributor Author

rtritto commented Sep 29, 2024

Also we should check that esbuild plugin not broken with it

I tested on Windows side:

  • with Vite/Vike (esbuild in the hood):
    • with Node, using @elysia/node, I get:

      Error [ERR_UNSUPPORTED_ESM_URL_SCHEME]: Only URLs with a scheme in: file, data, and node are supported by the default ESM loader. On Windows, absolute paths must be valid file:// URLs. Received protocol 'c:'

      We should change:

      const file = await import(fullPath);

      - const file = await import(fullPath);
      + import { pathToFileURL } from 'node:url';
      + const fileURL = pathToFileURL(fullPath);
      + const file = await import(fileURL.href);

      Should we do the change in this PR?

    • with Bun, I have no issue

  • without Vite/Vike, I have no issues:
    • with Node + @elysia/node
    • with Bun

@rtritto
Copy link
Contributor Author

rtritto commented Sep 29, 2024

I added @types/node dev dependency because globSync (Node v22) isn't recognized.
@types/bun also include @types/node but is v20 (Node v20).
After oven-sh/bun#13987 is merged, @types/node dev dependency can be removed.

@kravetsone
Copy link
Owner

Also we should check that esbuild plugin not broken with it

I tested on Windows side:

  • with Vite/Vike (esbuild in the hood):

    • with Node, using @elysia/node, I get:

      Error [ERR_UNSUPPORTED_ESM_URL_SCHEME]: Only URLs with a scheme in: file, data, and node are supported by the default ESM loader. On Windows, absolute paths must be valid file:// URLs. Received protocol 'c:'
      

      We should change:

      const file = await import(fullPath);

      - const file = await import(fullPath);
      + import { pathToFileURL } from 'node:url'
      + const fileURL = pathToFileURL(fullPath);
      + const file = await import(fileURL.href);

      Should we do the change in this PR?

    • with Bun, I have no issue

  • without Vite/Vike, I have no issues:

    • with Node + @elysia/node
    • with Bun

based i see this issue in my https://github.com/gramiojs/autoload too

Bun just works fine

@kravetsone
Copy link
Owner

Also we should check that esbuild plugin not broken with it

did u try it?
https://github.com/kravetsone/esbuild-plugin-autoload

But for now it uses Bun.Glob api (only in Bun.build step)

@rtritto
Copy link
Contributor Author

rtritto commented Oct 2, 2024

based i see this issue in my https://github.com/gramiojs/autoload too

Bun just works fine

I know.
Bun maybe wraps always (Windows or not) the parameter of import with pathToFileURL and get href.
Anyway the fix of dynamic import in Windows (maybe with pathToFileURL will be done in another PR (this PR is related to Node support)).
Opened #24 to discuss.

@rtritto
Copy link
Contributor Author

rtritto commented Oct 2, 2024

Also we should check that esbuild plugin not broken with it

did u try it? https://github.com/kravetsone/esbuild-plugin-autoload

But for now it uses Bun.Glob api (only in Bun.build step)

Not yet

@rtritto
Copy link
Contributor Author

rtritto commented Oct 2, 2024

How can we test with esbuild?

It that https://github.com/kravetsone/esbuild-plugin-autoload?tab=readme-ov-file#esbuild-usage the correct reference?

Draft:

await esbuild
  .build({
    target: "bun",
    outdir: "out",
    plugins: [{
      name: 'elysia-autoload',
      setup: autoload
    }]
  })
  .then(console.log)

@rtritto
Copy link
Contributor Author

rtritto commented Oct 9, 2024

Changed to use glob with async iteration with streams.
If needed, sortByNestedParams function can be restored and used to paths array (after for async).

const paths: [path: string, exportName: string][] = [];

@rtritto
Copy link
Contributor Author

rtritto commented Oct 10, 2024

This logic can be restored:

  const paths: [path: string, exportName: string][] = [];

  for await (const filePath of files) {
  // ...
-    if (types) paths.push([fullPath.replace(directoryPath, ""), importName]);
+    paths.push([fullPath.replace(directoryPath, ""), importName]);
  }

+  if (failGlob && files.length === 0)
+    throw new Error(
+      `No matches found in ${directoryPath}. You can disable this error by setting the failGlob parameter to false in the options of autoload plugin`,
+    );

@kravetsone what do you think about?

@kravetsone
Copy link
Owner

This logic can be restored:

  const paths: [path: string, exportName: string][] = [];

  for await (const filePath of files) {
  // ...
-    if (types) paths.push([fullPath.replace(directoryPath, ""), importName]);
+    paths.push([fullPath.replace(directoryPath, ""), importName]);
  }

+  if (failGlob && files.length === 0)
+    throw new Error(
+      `No matches found in ${directoryPath}. You can disable this error by setting the failGlob parameter to false in the options of autoload plugin`,
+    );

@kravetsone what do you think about?

Idk why u drop it

@rtritto
Copy link
Contributor Author

rtritto commented Oct 10, 2024

Because I changed the glob scan to async iteration with streams (https://bun.sh/docs/api/glob#quickstart) and the filepath list can't be collected.

@kravetsone
Copy link
Owner

Because I changed the glob scan to async iteration with streams (https://bun.sh/docs/api/glob#quickstart) and the filepath list can't be collected.

It is possible to add let isEmpty and in for if it false set to true
And u will know if generator return zero results
I guess maybe a better way exists but this way is okay too

@kravetsone
Copy link
Owner

Changed to use glob with async iteration with streams.
If needed, sortByNestedParams function can be restored and used to paths array (after for async).

const paths: [path: string, exportName: string][] = [];

I add sortByNestedParams to prevent some/[ok] be higher than some/ok

Because path with params should be connected in last way

@rtritto
Copy link
Contributor Author

rtritto commented Oct 10, 2024

Changed to use glob with async iteration with streams.
If needed, sortByNestedParams function can be restored and used to paths array (after for async).

const paths: [path: string, exportName: string][] = [];

I add sortByNestedParams to prevent some/[ok] be higher than some/ok

Because path with params should be connected in last way

Ok, I restored the previous logic.

@kravetsone
Copy link
Owner

is this PR ready for merge?

@kravetsone
Copy link
Owner

Do we really need esbuild?

I guess we can recommend tsx to execute TS on NodeJS

It is also recommended in NodeJS documentation and it has support for import(".ts")

@kravetsone
Copy link
Owner

love this PR

also when elysia going to support node much better it really good

@rtritto rtritto marked this pull request as ready for review October 27, 2024 10:08
@rtritto
Copy link
Contributor Author

rtritto commented Oct 27, 2024

is this PR ready for merge?

I don't know if we should add a message error when using NodeJS and missing esbuild.
Anyway README needs to be updated with NodeJS usage case (install esbuild when using .ts files etc).

Note: on merging this PR, I would use the squash (cleaner history) instead of the merge.

Do we really need esbuild?

NodeJS doesn't support .ts files. esbuild is an optional peer dependency.

I guess we can recommend tsx to execute TS on NodeJS

It is also recommended in NodeJS documentation and it has support for import(".ts")

tsx uses esbuild under the hood and esbuild is used widely (so it can be easily deduped and we have essentials dependencies).
If there is a pereferred (usefull) setting in tsx, we can copy it.

@kravetsone
Copy link
Owner

is this PR ready for merge?

I don't know if we should add a message error when using NodeJS and missing esbuild.
Anyway README needs to be updated with NodeJS usage case (install esbuild when using .ts files etc).

Note: on merging this PR, I would use the squash (cleaner history) instead of the merge.

Do we really need esbuild?

NodeJS doesn't support .ts files. esbuild is an optional peer dependency.

I guess we can recommend tsx to execute TS on NodeJS

It is also recommended in NodeJS documentation and it has support for import(".ts")

tsx uses esbuild under the hood and esbuild is used widely (so it can be easily deduped and we have essentials dependencies).
If there is a pereferred (usefull) setting in tsx, we can copy it.

I mean that tsx is de-facto standard and it mentioned in NodeJS documentation

So if some TypeScript executor isn't support .ts files it will be not our problem)
I guess no need to use esbuild directly in plugin
It's more about executor scope

@rtritto
Copy link
Contributor Author

rtritto commented Oct 27, 2024

I mean that tsx is de-facto standard and it mentioned in NodeJS documentation

So if some TypeScript executor isn't support .ts files it will be not our problem)
I guess no need to use esbuild directly in plugin
It's more about executor scope

In the doc, tsx is used only with scripts (shell).
In our use case, we can use directly esbuild like vite and vike projects.
Another point where tsx (currently) uses esbuild@~0.23.0 and latest esbuild version is 0.24.0. Using directly esbuild as dev dependency, the user can choose which version to use (in case of problems or every kind of updates).

@kravetsone
Copy link
Owner

I mean that tsx is de-facto standard and it mentioned in NodeJS documentation

So if some TypeScript executor isn't support .ts files it will be not our problem)
I guess no need to use esbuild directly in plugin
It's more about executor scope

In the doc, tsx is used only with scripts (shell).
In our use case, we can use directly esbuild like vite and vike projects.
Another point where tsx (currently) uses esbuild@~0.23.0 and latest esbuild version is 0.24.0. Using directly esbuild as dev dependency, the user can choose which version to use (in case of problems or every kind of updates).

Didn't understand what u mean by "we can use directly" What's we get with it?

Seems like elysia application will be bundled or executed by tsx

@rtritto
Copy link
Contributor Author

rtritto commented Oct 28, 2024

Didn't understand what u mean by "we can use directly" What's we get with it?

esbuild is a bundler and it's used to build and transpile.
tsx is used, in place of node command, to execute code (shell commands, package.json scripts etc) with a loader. Under the hood it uses esbuild to transpile and then execute.
With "directly" I mean that we can use the build function of esbuild instead of tsx that uses the build function of esbuild.

Seems like elysia application will be bundled or executed by tsx

Elysia is bundled with tsup (an alternative to pkgroll and that uses esbuild) and then with Bun (source). It's executed by Bun (Bun runtime with TypeScript).


Some differences: https://dev.to/andreasbergstrom/simplify-typescript-builds-with-esbuild-and-skip-tsctsx-2124

Some references:

@kravetsone
Copy link
Owner

kravetsone commented Oct 28, 2024

yeah tsx is executor
esbuild is bundler

and classic user flow will be:

  1. use tsx to execute typescript files (works with elysia-autoload because import(".ts") supported)
  2. transpile it via bundler (works with elysia-autoload because files transpiled to .js)

So why we need to execute esbuild directly?

@rtritto
Copy link
Contributor Author

rtritto commented Oct 29, 2024

When you start vite and vike, you don't use tsx because they use esbuild internally. At same way, elysia-autoload should use esbuild.
Use tsx when you need to start .ts (execute a file with no exports).
Use esbuild when you internally check the file extension and you can select the default or named export(s) to execute or use.

@rtritto rtritto marked this pull request as draft December 3, 2024 13:03
@rtritto
Copy link
Contributor Author

rtritto commented Dec 3, 2024

@kravetsone the error TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".ts" for /<FILE>.ts, with the dynamic import on Node.js for .ts and .tsx files, has a related issue with Vite: vitejs/vite#5370

As probably Bun do, a loader needs to be used on entrypoint.

So, I did a commit to remove esbuild.

Imo we can always use pathToFileURL (same as #30):

const file = await import(pathToFileURL(fullPath).href)

instead of

let file;
if (typeof Bun === "undefined")
  ? await import(pathToFileURL(fullPath).href)
  : await import(fullPath)

What do you think about?

@rtritto
Copy link
Contributor Author

rtritto commented Dec 3, 2024

My comment: vitejs/vite#5370 (comment)

@kravetsone kravetsone marked this pull request as ready for review December 28, 2024 21:11
@kravetsone kravetsone merged commit f96b0a8 into kravetsone:main Dec 28, 2024
@kravetsone
Copy link
Owner

Thanks for the work! Love it

For final i guess we should not include any loaders to this library.
We should talk to user about this.

Also tsx is a great solution and no problem will be caused by this.
And type stripping in new Node.js versions will be fine!

@rtritto rtritto deleted the node-support branch January 14, 2025 00:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants