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

invalid moduleId in a TypeScript monorepo #235

Closed
tpluscode opened this issue Sep 6, 2023 · 35 comments · Fixed by #243
Closed

invalid moduleId in a TypeScript monorepo #235

tpluscode opened this issue Sep 6, 2023 · 35 comments · Fixed by #243

Comments

@tpluscode
Copy link

tpluscode commented Sep 6, 2023

I have a monorepo with two packages rdf-web-access-control and hydra-box-web-access-control. The latter imports the former

import { check } from 'rdf-web-access-control'

In tests I want to mock this import

const aclSpy = await esmock('../index.ts', {
  'rdf-web-access-control': {
    check: sinon.stub(),
  },
})

This fails:

Error: invalid moduleId: "rdf-web-access-control" (used by file:///<root>/packages/hydra-box-web-access-control/test/index.test.ts)
    at Object.Ee [as errModuleIdNotFound] (file:///<root>/node_modules/esmock/src/esmock.js:1:61)
    at K (file:///<root>/node_modules/esmock/src/esmock.js:2:5883)
    at wt (file:///<root>/node_modules/esmock/src/esmock.js:2:6379)
    at file:///<root>/node_modules/esmock/src/esmock.js:3:161
    at async Context.<anonymous> (file:///<root>/packages/hydra-box-web-access-control/test/index.test.ts:34:14)
@koshic
Copy link
Collaborator

koshic commented Sep 6, 2023

Could you please provide a reproducible scenario (which is much more important for TS projects due to additional TS->JS layer)?

@iambumblehead
Copy link
Owner

@tpluscode if you share more of a reproduction we could look for the solution.

https://github.com/iambumblehead/esmock/wiki#call-esmock-typescript

For some typescript scenarious, parentUrl must be defined explicitly so that a moduleId such as "rdf-web-access-control" can be located relative to the correct parent location.

@tpluscode
Copy link
Author

tpluscode commented Sep 6, 2023

There: https://github.com/tminuscode/esmock-invalid-module-id

Here's the tl;dr;

import assert from 'assert'
import esmock from 'esmock'
+import module from 'module'

describe('repro', function () {
    it('works', async () => {
+       const require = module.createRequire(import.meta.url)
+
        const sut = await esmock('./index.ts', {
-           a: {
+           [require.resolve('a')]: {
                foo() {
                    return 'bar'
                }
            }
        })

        assert.equal(sut(), 'bar')
    })
})

As you see, I found that resolving the module up-front is a workaround.

As a side note, I expected esmock('./index.js') to work since that is how typescript resolves modules, not by .ts

@tpluscode
Copy link
Author

And no, import.meta.url on second arg does not make a difference

@iambumblehead
Copy link
Owner

@tpluscode thanks. Some radical changes in node 20.6 are using my time #234 there might be some delay before we can resolve this typescript monorepo issue you've opened.

@tpluscode
Copy link
Author

No worries. Thankfully, there is a workaround 🙏

@iambumblehead
Copy link
Owner

resolvewithplus does not support workspaces https://github.com/iambumblehead/resolvewithplus

That is a nice solution with module.createRequire(import.meta.url) and I wonder what the best way of integrating that with resolvewithplus might be.

@iambumblehead
Copy link
Owner

On further thought...

It is risky to build on top of things defined by the node runtime because those things are frequently removed, changed and broken. Nothing is stable there for any duration of time.

It might be best for all users to use your solution as demonstrated in your example. When module.createRequire disappears or changes, users can handily switchover to another approach without being obstructed by esmock's reliance on that.

@iambumblehead
Copy link
Owner

This approach is mentioned in the wiki https://github.com/iambumblehead/esmock/wiki#call-esmock-npm-workspaces thank you. If there are no objections over the next few days, this issue will be closed

@tpluscode
Copy link
Author

And what about the esmock('./index.js') vs esmock('./index.ts')? Why doesn't the former work?

@iambumblehead
Copy link
Owner

I have less familiarity with typescript. I would like to know, when a typescript application calls esmock('./index.js') does index.js exist in the filesystem? Eg, does a resolver need to locate 'index.js' or does it need to locate index.ts from using moduleId 'index.js'?

@iambumblehead
Copy link
Owner

@tommy-mitchell you are also a typescript user and it would be nice to read any thoughts you may have re this last issue

@iambumblehead
Copy link
Owner

There could be circumstances where both a ".ts" and a ".js" file exist. Additionally, both file types could possibly resolve at different steps and in different locations of the lookup sequence. Though I don't often use typescript, returning "index.ts" to the moduleId "index.js" seems inherently confusing.

@tpluscode
Copy link
Author

when a typescript application calls esmock('./index.js') does index.js exist in the filesystem?

No, typically only index.ts exists

returning "index.ts" to the moduleId "index.js" seems inherently confusing.

In TS, all ESM imports should have the extension .js because the compiler does not do any processing of import statements during transpilation. Thus, ts-node/esm (and, I assume, other loaders) resolve bar.ts when you have import foo from './bar.js.

There could be circumstances where both a ".ts" and a ".js" file exist

Yes, if both files exist, such as after compiling with tsc, the compiled JS will be found first. This has always been a little gotcha of running TS from sources, possibly leading to loading outdated code being run.

@iambumblehead
Copy link
Owner

iambumblehead commented Sep 8, 2023

@tpluscode thanks for the thoughtful and informative reply. Currently, the resolver follows the node esm resolver sequence, which does not resolve .ts files from .js moduleIds.

Maybe esmock could somehow detect the use of typescript and do something like...

const modulepath = istypescript
  ? resolve('file:///to/moduleId.js') || resolve('file:///to/moduleId.ts')
  : resolve('file:///to/moduleId.js')

@koshic
Copy link
Collaborator

koshic commented Sep 8, 2023

In TS, all ESM imports should have the extension .js

Not necessary, you can use https://www.typescriptlang.org/tsconfig#allowImportingTsExtensions

@koshic
Copy link
Collaborator

koshic commented Sep 8, 2023

@tpluscode sadly, your project doesn't work even on pure js:
image

➜  b git:(master) ✗ node index.js
node:internal/modules/esm/resolve:188
  const resolvedOption = FSLegacyMainResolve(packageJsonUrlString, packageConfig.main, baseStringified);
                         ^

Error: Cannot find package '/xxx/esmock-invalid-module-id/node_modules/a/package.json' imported from /xxx/esmock-invalid-module-id/packages/b/index.js
    at legacyMainResolve (node:internal/modules/esm/resolve:188:26)
    at packageResolve (node:internal/modules/esm/resolve:769:14)
    at moduleResolve (node:internal/modules/esm/resolve:831:20)
    at defaultResolve (node:internal/modules/esm/resolve:1036:11)
    at DefaultModuleLoader.resolve (node:internal/modules/esm/loader:251:12)
    at DefaultModuleLoader.getModuleJob (node:internal/modules/esm/loader:140:32)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:76:33)
    at link (node:internal/modules/esm/module_job:75:36) {
  code: 'ERR_MODULE_NOT_FOUND'
}

Node.js v20.5.0

Reason - non-existent 'index.js' in main field:
image

package.json main / exports mechanics is Node responsibility (and have well defined algorithm for third-party resolvers), so TS-related magic doesn't work here.

Just change it to '.ts' and everything works fine:
image

PS if you need '.js' exports for some purposes - you can use conditional exports: 'import' -> 'index.js' (default way) and something like 'test' -> 'index.ts' then run mocha with 'test' condition defined. Or just use 'publishConfig' field.

@tpluscode
Copy link
Author

Not necessary, you can use https://www.typescriptlang.org/tsconfig#allowImportingTsExtensions

Yes, I was aware. This only works when the code is not transpiled. If you want to publish JS code from a TypeScript project, or compile to run without TS loader in runtime, you will need to use .js

sadly, your project doesn't work even on pure js:

I don't understand what you did there. Here's a branch with minimal changes showing that this exact setup works fine without TS: https://github.com/tminuscode/esmock-invalid-module-id/tree/js

You cannot change main field in package to JS because it will stop working once a package in compiled and published to NPM. And no, it is not necessary.

PS if you need '.js' exports for some purposes

Yes, indeed. That purpose being that others can use my library 😂

Conditional exports are not intended to be used with TS. They exist to cater for dual CJS+ESM packages. TypeScript sources, however, are not typically included with the package on NPM.

@iambumblehead
Copy link
Owner

Thanks for spending energy on this good discussion. These are interesting things to consider.

Some other issues are still using my time but discussion and conclusions here are going to be helpful.

@koshic
Copy link
Collaborator

koshic commented Sep 8, 2023

If you want to publish JS code from a TypeScript project, or compile to run without TS loader in runtime, you will need to use .js

No ) '.js' is required only if you want to use tsc as compiler. For all other tools like esbuild, swc, babel or even tsc as type checker you can use '.ts'.

You cannot change main field in package to JS because it will stop working once a package in compiled and published to NPM.

You can ) To solve issues like that, when published package has a different entry point (like 'dist/index.js' instead of 'src/index.js') you can use 'publishConfig' field, supported by all package managers - https://docs.npmjs.com/cli/v8/configuring-npm/package-json#publishconfig.

Small example (real code from one of my projects):

  "imports": {
    "#utils/*": "./src/utils/*.ts",
    "#load": "./src/load/hook.ts",
    "#resolve": "./src/resolve/hook.ts",
    "#src/*": "./src/*.ts"
  },
  "exports": "./dev/loader.js",
  "files": [
    "dist"
  ],
  "publishConfig": {
    "imports": {
      "#utils/*": "./dist/utils/*.js",
      "#load": "./dist/load/hook.js",
      "#resolve": "./dist/resolve/hook.js"
    },
    "exports": "./dist/loader.js"
  },

That purpose being that others can use my library

I mean some libraries may be 'internal' part of monorepo which is not published directly.

Conditional exports are not intended to be used with TS. They exist to cater for dual CJS+ESM packages

It's not quite true. According to https://nodejs.org/dist/latest-v20.x/docs/api/packages.html#conditional-exports, "Conditional exports provide a way to map to different paths depending on certain conditions". Moreover, TS support types export trough special condition 'types', https://www.typescriptlang.org/docs/handbook/esm-node.html#packagejson-exports-imports-and-self-referencing.

What I want to say - those conditions can be used (and already used!) for various tasks when package should provide different exports depending on some conditions. You can treat them as a replacement for various non-standard fields like 'browser', 'module, 'types' which force developers to reimplement package.json parsing again and again while Node resolver supports single 'main' field.

@iambumblehead
Copy link
Owner

you can use 'publishConfig' field

interesting did not know this!

@tpluscode
Copy link
Author

Those are some fancy configurations, very interesting. In fact, the publish config may solve my issue when referencing bin of another package within a monorepo.

Otherwise, I would still stick to the defaults which do not need any hokey pokey. Importing .js "just works" in 99% percent of scenarios :)

@iambumblehead
Copy link
Owner

possible workspaces-related issue #241 (comment)

@iambumblehead
Copy link
Owner

@tpluscode @koshic esmock should support workspaces. I'll make a PR to add workspaces support to resolvewithplus soonish. Support could be added without using module.createRequire

@iambumblehead
Copy link
Owner

@tpluscode thanks for opening this issue and finding ways to make esmock work despite the short-comings.

@iambumblehead
Copy link
Owner

@tpluscode the sample repo you made looks terrific and clearly demonstrates how workspaces are used

@iambumblehead
Copy link
Owner

iambumblehead/resolvewithplus#46
#243

Workspace-related branches are created with tests. The error can probably be reproduced there soon.

@iambumblehead
Copy link
Owner

https://github.com/tminuscode/esmock-invalid-module-id does not surface an invalid module id error here. Instead the error output looks like this,

$ npm test

> test
> npm --prefix packages/b test


> [email protected] test
> mocha test.ts

[warning messages removed]


  repro
    ✔ works ootb (148ms)
    1) works roubdabout way


  1 passing (157ms)
  1 failing

  1) repro
       works roubdabout way:
     Error: Cannot find module '/home/bumble/software/esmock-invalid-module-id/node_modules/a/index.js'. Please verify that the package.json has a valid "main" entry
      at tryPackage (node:internal/modules/cjs/loader:415:19)
      at Module._findPath (node:internal/modules/cjs/loader:665:18)
      at Module._resolveFilename (node:internal/modules/cjs/loader:1034:27)
      at Function.resolve (node:internal/modules/helpers:136:19)
      at Context.<anonymous> (file:///home/bumble/software/esmock-invalid-module-id/packages/b/test.ts:18:22)
      at process.processImmediate (node:internal/timers:478:21)

the test completes successfully if the main definition is updated to reference "index.ts" rather than "index.js"

diff --git a/packages/a/package.json b/packages/a/package.json
index e9ec9a1..fd9b409 100644
--- a/packages/a/package.json
+++ b/packages/a/package.json
@@ -2,5 +2,5 @@
   "name": "a",
   "version": "0.0.0",
   "type": "module",
-  "main": "index.js"
+  "main": "index.ts"
 }

@tpluscode https://github.com/tminuscode/esmock-invalid-module-id what error is expected from this repo? if you are seeing the invalid module id error, what should I do to see that error?

@tpluscode
Copy link
Author

Maybe runtime version difference?

I get that error with node 18.17.1 and 16.19

On 20.4 I actually have both tests fail.

@iambumblehead
Copy link
Owner

Node v16 cannot be used to chain loaders, so esmock and ts-node/esm will not work there. I was able to reproduce the errors with node v18 Error: invalid moduleId: "a" and the error does relate to resolving of ".js" moduleId to ".ts" files paths.

@tpluscode @koshic what do you think of this plan? If the parent file has a ".ts" extension, such as "b/test.ts", the resolver detects this and uses typescript conditions to return, for example, "index.ts" from "index.js"

const parent = 'file:///esmock-invalid-module-id/packages/b/test.ts'
// resolver detects ".ts" extension on the parent and applies typescript conditions
// typescript conditions return "a/index.ts" before "a/index.js"
const moduleFileURL = resolvewith('a', parent) // 'file:///esmock-invalid-module-id/packages/a/index.ts'

@iambumblehead
Copy link
Owner

@tpluscode using the newest resolver changes, the test passes using esmock('./index.js') so possibly all issues reported here will be resolved soon. When changes can be verified with a ts-style workspace test to at the PR ... let's try using that.

@iambumblehead
Copy link
Owner

@tpluscode please verify if this branch is works correctly for you #243

One way to specify this branch is to use a git url like this one,

  "esmock": "https://github.com/iambumblehead/esmock.git#add-workspace-tests"

The new branch uses an updated version of resolvewithplus and should do the following things,

  • when typescript is detected, should resolve moduleIds from things like { "main": "./index.js" }, where "index.ts" should be returned rather than "index.js",
  • when typescript is detected, should resolve moduleIds from things like "./local-file.js" where "./local-file.ts" should be returned rather than "./local-file.js"

@tpluscode feel free to leave any comments and to review the branch #243. A significant change made to esmock; the tests included a "main.js" and a "main.ts" file and "main.ts" was renamed "main-ts.ts" because the resolver started to return "main.js" where the tests expected "main.ts". Based on what you've written, it is correct for esmock to resolve "main.js" from the moduleId "main.ts"... so everything seems good.

@tpluscode
Copy link
Author

Node v16 cannot be used to chain loaders

I could swear it works in later v16. Maybe they backported chained loaders? As that feature was also unavailable in past v18 branch

I will try to review the PR changes soon. Thank you for working on this despite the initial decision 🙇

@iambumblehead
Copy link
Owner

iambumblehead commented Sep 13, 2023

https://nodejs.org/en/blog/release/v16.17.0

Node.js ESM Loader hooks now support multiple custom loaders, and composition is achieved via "chaining

I see you are right :) both tests from your repo pass here using this branch with node v16.20.2

@tpluscode
Copy link
Author

Or just use 'publishConfig' field.

Actually @koshic, how is that supposed to work? I tried to override fields like main, bin or exports with publishConfig but they are always the original values when I npm pack or npm publish (for the latter I experimented locally with verdaccio)

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 a pull request may close this issue.

3 participants