Skip to content

fix(rspeedy/core): deno compat #1412

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Aug 5, 2025
Merged

fix(rspeedy/core): deno compat #1412

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d3bbd91
fix rspeedy deno compat
knotbin Aug 4, 2025
0e44566
hasNativeTSSupport deno logic
knotbin Aug 4, 2025
db0b806
Merge branch 'lynx-family:main' into main
knotbin Aug 4, 2025
64ce3fd
remove bailout in start, conditional unregister
knotbin Aug 4, 2025
8a1cd35
Merge branch 'lynx-family:main' into main
knotbin Aug 5, 2025
c59e9e7
fix js support
knotbin Aug 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/clear-candies-start.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@lynx-js/rspeedy": patch
---

fix deno compatibility
10 changes: 10 additions & 0 deletions packages/rspeedy/core/register/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,16 @@ import module from 'node:module'
* @returns - The `unregister` function.
*/
export function register(options) {
// Check for Deno environment first
Copy link
Collaborator

@colinaaa colinaaa Aug 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have a hasNativeTSSupport function at:

lynx-stack/packages/rspeedy/core/src/config/loadConfig.ts

Line 155 in 15a46bc

function hasNativeTSSupport(): boolean {

which is based on process.features.typescript.

Although deno does not support process.features.typescript yet :)

Image

But it would be great to have the similar logic together.

Copy link
Contributor Author

@knotbin knotbin Aug 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried just adding the Deno logic to hasNativeTSSupport but it resulted in the same error when trying to run with Deno because the current logic assumes nativeTsSupport runtimes still support module.register.

I ended up using hasNativeTSSupport in a conditional for running register.

This only meant I had to change the commonConfig.js imports to commonConfig.ts in web-tests because these specific tests are run directly from the TypeScript source and not compiled into JS.

All other tests and libraries built perfectly fine with no changes and all tests pass.

I understand this change may be too much for just adding Deno support and I really did want to keep it isolated but that would have meant using the original change or making the conditional specific to only Deno. Please let me know if this meets your standards.

Copy link
Collaborator

@colinaaa colinaaa Aug 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are using register without condition to fix #1406.

I do think it would be necessary to keep this way since there could be a lot of user that are doing this way (import with .js for a .ts file) since this is the default of TypeScript.

Copy link
Collaborator

@colinaaa colinaaa Aug 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer something like this:

  const unregister = shouldUseNativeImport(configPath)
    ? /** noop */ () => void 0
    : register({
      load: !hasNativeTSSupport(),
      resolve: true,
    })


function shouldUseNativeImport(configPath: string): boolean {
  return isJavaScriptPath(configPath) || isDeno()
}

function isDeno(): boolean {
  // @ts-expect-error - Deno global may not be defined in Node.js
  if (typeof Deno !== 'undefined' || process.versions?.deno) {
    return true
  }
  return false
}

wdyt

Copy link
Contributor Author

@knotbin knotbin Aug 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a perfect solution! Sorry I tried a bunch of combinations of isJavaScriptPath, hasNativeTSSupport and the Deno logic but this one is clearly the most logical. Just updated it to reflect this and removed the web-test changes. Let me know what you think.

// @ts-expect-error - Deno global isn't defined in Node.js
if (typeof Deno !== 'undefined' || process.versions?.deno) {
// Deno doesn't support module.register(), but it has native TypeScript support
// Return a no-op unregister function
return function unregister() {
// No-op for Deno
}
}

// eslint-disable-next-line n/no-unsupported-features/node-builtins
if (!module.register) {
throw new Error(
Expand Down
6 changes: 6 additions & 0 deletions packages/rspeedy/core/src/cli/start.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,12 @@ function tryGetPackageFolderFor(
export function tryStartLocalRspeedy(
root: string = process.cwd(),
): false | Promise<void> {
// Check for Deno environment and automatically use unmanaged mode
// @ts-expect-error - Deno global isn't be defined in Node.js
if (typeof Deno !== 'undefined' || process.versions?.deno) {
return false
}

if (process.argv.includes(Constants.unmanagedParameterLongName)) {
logger.info(
`Bypassing the Rspeedy version selector because ${
Expand Down