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

node compat: Dynamic import leads to missing globals #20028

Closed
marvinhagemeister opened this issue Aug 2, 2023 · 8 comments · Fixed by #20153
Closed

node compat: Dynamic import leads to missing globals #20028

marvinhagemeister opened this issue Aug 2, 2023 · 8 comments · Fixed by #20153
Labels
bug Something isn't working correctly node compat

Comments

@marvinhagemeister
Copy link
Contributor

A dynamic import that is not statically analysable breaks npm detection which leads to node globals missing.

This was originally reported on the Fresh repo denoland/fresh#1577 . In Fresh the dev.ts entry point calls a dynamic import where the specifier is not statically analysable:

export async function dev(base: string, entrypoint: string) {
  entrypoint = new URL(entrypoint, base).href;
  // ...
  await import(entrypoint);
}

Reproduction repo: https://github.com/marvinhagemeister/deno-npm-bug

Steps to reproduce

  1. Clone https://github.com/marvinhagemeister/deno-npm-bug
  2. Run deno run -A foo.ts
@bartlomieju
Copy link
Member

CC @lucacasonato

@bartlomieju
Copy link
Member

bartlomieju commented Aug 2, 2023

The contents of the file are:

"use strict";
/**
 * Node.js implementation of browser `btoa` function.
 *
 * @packageDocumentation
 * @internal
 */
Object.defineProperty(exports, "__esModule", { value: true });
exports.base64Encode = void 0;
/**
 * @internal
 */
function base64Encode(str) {
    return Buffer.from(str).toString("base64");
}
exports.base64Encode = base64Encode;
//# sourceMappingURL=btoa.js.map⏎

I think this might be the limitation Luca mentioned, but I'm not sure.

@lucacasonato
Copy link
Member

No, this doesn't look related. Please bisect if you think it's related to my globals PR. I think what is happening is that internals.node.initialize() is either not being called, or there is a race. Can you find that out?

I am about 98% confident this bug was already present when I implemented npm: support in Fresh. I reported it to @dsherret back then.

@bartlomieju
Copy link
Member

Ah, thanks for the pointer, that's probably related to #15826 then.

@loynoir
Copy link

loynoir commented Aug 3, 2023

Minimal reproduce.

Seems like something depends on simple AST, actually.

But should implement as AST-independent.

OK when 'id'

==> foo.test.ts <==
import kleur from 'npm:kleur@3.0.0'

Deno.test('x', () => {
  console.log({ kleur })
  kleur.red('1x')
})

==> test.ts <==
await import('./foo.test.ts')

Error when `id`

==> foo.test.ts <==
import kleur from 'npm:kleur@3.0.0'

Deno.test('x', () => {
  console.log({ kleur })
  kleur.red('1x')
})

==> test.ts <==
await import(`./foo.test.ts`)

@williamhorning
Copy link

I've been able to reproduce this this with both Deno 1.35.3 and 1.36.0 and found that the globals for Node packages are set right if you statically import those packages before they are dynamically imported. williamhorning/deno-dynamic-import-globals has an example of this using undici.

@davideperozzi
Copy link

@williamhorning Thank you! I've been looking for a temporary fix. Importing any npm module before the dynamic import did the trick. So I'm basically doing import 'npm:is-number' (just using it, becaus it's small) to force the globals to load.

@bartlomieju
Copy link
Member

FYI you should be able to work-around the problem by importing any built-in node: module, no need to import npm: libraries.

williamhorning added a commit to williamhorning/bolt that referenced this issue Aug 6, 2023
mmastrac added a commit that referenced this issue Aug 15, 2023
To fix bugs around detection of when node emulation is required, we will
just eagerly initialize it. The improvements we make to reduce the
impact of the startup time:

 - [x] Process stdin/stdout/stderr are lazily created
 - [x] node.js global proxy no longer allocates on each access check
- [x] Process checks for `beforeExit` listeners before doing expensive
shutdown work
- [x] Process should avoid adding global event handlers until listeners
are added

Benchmarking this PR (`89de7e1ff`) vs main (`41cad2179`)

```
12:36 $ third_party/prebuilt/mac/hyperfine --warmup 100 -S none './deno-41cad2179 run ./empty.js' './deno-89de7e1ff run ./empty.js'
Benchmark 1: ./deno-41cad2179 run ./empty.js
  Time (mean ± σ):      24.3 ms ±   1.6 ms    [User: 16.2 ms, System: 6.0 ms]
  Range (min … max):    21.1 ms …  29.1 ms    115 runs
 
Benchmark 2: ./deno-89de7e1ff run ./empty.js
  Time (mean ± σ):      24.0 ms ±   1.4 ms    [User: 16.3 ms, System: 5.6 ms]
  Range (min … max):    21.3 ms …  28.6 ms    126 runs
```

Fixes #20142
Fixes #15826
Fixes #20028
littledivy pushed a commit to littledivy/deno that referenced this issue Aug 21, 2023
To fix bugs around detection of when node emulation is required, we will
just eagerly initialize it. The improvements we make to reduce the
impact of the startup time:

 - [x] Process stdin/stdout/stderr are lazily created
 - [x] node.js global proxy no longer allocates on each access check
- [x] Process checks for `beforeExit` listeners before doing expensive
shutdown work
- [x] Process should avoid adding global event handlers until listeners
are added

Benchmarking this PR (`89de7e1ff`) vs main (`41cad2179`)

```
12:36 $ third_party/prebuilt/mac/hyperfine --warmup 100 -S none './deno-41cad2179 run ./empty.js' './deno-89de7e1ff run ./empty.js'
Benchmark 1: ./deno-41cad2179 run ./empty.js
  Time (mean ± σ):      24.3 ms ±   1.6 ms    [User: 16.2 ms, System: 6.0 ms]
  Range (min … max):    21.1 ms …  29.1 ms    115 runs
 
Benchmark 2: ./deno-89de7e1ff run ./empty.js
  Time (mean ± σ):      24.0 ms ±   1.4 ms    [User: 16.3 ms, System: 5.6 ms]
  Range (min … max):    21.3 ms …  28.6 ms    126 runs
```

Fixes denoland#20142
Fixes denoland#15826
Fixes denoland#20028
littledivy pushed a commit that referenced this issue Aug 21, 2023
To fix bugs around detection of when node emulation is required, we will
just eagerly initialize it. The improvements we make to reduce the
impact of the startup time:

 - [x] Process stdin/stdout/stderr are lazily created
 - [x] node.js global proxy no longer allocates on each access check
- [x] Process checks for `beforeExit` listeners before doing expensive
shutdown work
- [x] Process should avoid adding global event handlers until listeners
are added

Benchmarking this PR (`89de7e1ff`) vs main (`41cad2179`)

```
12:36 $ third_party/prebuilt/mac/hyperfine --warmup 100 -S none './deno-41cad2179 run ./empty.js' './deno-89de7e1ff run ./empty.js'
Benchmark 1: ./deno-41cad2179 run ./empty.js
  Time (mean ± σ):      24.3 ms ±   1.6 ms    [User: 16.2 ms, System: 6.0 ms]
  Range (min … max):    21.1 ms …  29.1 ms    115 runs
 
Benchmark 2: ./deno-89de7e1ff run ./empty.js
  Time (mean ± σ):      24.0 ms ±   1.4 ms    [User: 16.3 ms, System: 5.6 ms]
  Range (min … max):    21.3 ms …  28.6 ms    126 runs
```

Fixes #20142
Fixes #15826
Fixes #20028
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working correctly node compat
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants