Skip to content
This repository has been archived by the owner on Sep 16, 2023. It is now read-only.

loading hooks conflict #679

Closed
vijithassar opened this issue Nov 27, 2018 · 8 comments
Closed

loading hooks conflict #679

vijithassar opened this issue Nov 27, 2018 · 8 comments
Labels

Comments

@vijithassar
Copy link

Ideally it should be possible to compose esm alongside other loading hooks, but there seems to be a conflict between esm and lit-node, a module which allows Node to execute code extracted from Markdown code blocks. The Markdown code crashes if it includes ESM imports, even if the esm module is already loaded.

# Markdown Content

## CommonJS
This works :D
```javascript
const ok = require('assert').ok
ok(true)
```

## ESM
This crashes :(
```javascript
import { ok } from 'assert'
ok(true)
```

Here's a repository which explains the problem in more detail and includes tests.

@vijithassar
Copy link
Author

cc @Rich-Harris

@Rich-Harris
Copy link

Is this happening because lit-node wraps everything in an async IIFE (to crudely simulate top-level await)?

@vijithassar
Copy link
Author

That has already been identified as a possible issue, but I am not sure it's the problem here, because I still get the same crash if I remove the IIFE from a local copy of lit-node/compile.js.

@jdalton
Copy link
Member

jdalton commented Nov 27, 2018

Hi @vijithassar!

Thanks for the ping. It looks like lit-node is registering its own .md extension compiler. As such it is 💯 responsible for the processing of that extension. Since it isn't transpiling the ESM content when it hits the vm.Script it errors out. The esm loader does have a hook for vm.Script but doesn't enable it for the application level usage. I can look into enabling it though.

Update:

Ah ok, so on further testing there are two issues here. One on lit-node and one on esm. For lit-node the async wrapper would cause an issue since that would create a nested static import.... but there is something else here that esm should be doing but isn't. Since you're using module._compile() the esm loader should be hooked in there. This part looks like an esm issue.

@vijithassar
Copy link
Author

I've updated the test repository to use an updated version of lit-node which no longer wraps its transformed output code in a top-level async function.

@jdalton
Copy link
Member

jdalton commented Nov 27, 2018

Thanks @vijithassar! That leaves the 🐛 on esm.

Update:

So what's happening is there are some incorrect assumptions being done by esm. We setup the module object to complete its compile step through the .js or .mjs. If it's not we defer to the built-in compile functionality (so does not process ESM code). I believe I can tweak that assumption so that other registered extension handlers that also call module._compile can get their code translated.

Update:

I have this working locally.

Update:

Patch 0353dcc; Test 2341169.

@jdalton jdalton closed this as completed Dec 3, 2018
@vijithassar
Copy link
Author

The tests I submitted are still crashing for me, even when I clone the current version of esm from GitHub rather than installing the most recent version from npm.

$ node --require esm --require lit-node/register esm.md

path/node_modules/esm/esm.js:253
  const { dir } = shared.package
          ^

TypeError: Cannot destructure property `dir` of 'undefined' or 'null'.

@jdalton
Copy link
Member

jdalton commented Dec 13, 2018

Hi @vijithassar! You're gonna have to delete your node_modules/.cache. I've changed the cache data format. This won't be a problem when officially released as the version will be bumped and will trigger a cache invalidation automagically.

FWIW we do have a scenario test covering lit-node: 2341169

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

No branches or pull requests

3 participants