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

Issue when importing Mappersmith from a TypeScript file and using esbuild #414

Closed
manisenkov opened this issue Jan 5, 2024 · 8 comments · Fixed by #417
Closed

Issue when importing Mappersmith from a TypeScript file and using esbuild #414

manisenkov opened this issue Jan 5, 2024 · 8 comments · Fixed by #417

Comments

@manisenkov
Copy link

Hi!

I'm trying to bump one of our projects from Mappersmith v2.42 to v2.43.1 and encounter an issue with new ESM modules. We're using Node v16 and esbuild v0.19.11 to bundle the project into one file. But build failed with an error

✘ [ERROR] Could not resolve "./mappersmith"

    node_modules/mappersmith/esm/index.mjs:1:24:
      1 │ import { configs } from "./mappersmith";
        ╵                         ~~~~~~~~~~~~~~~

✘ [ERROR] Could not resolve "./gateway/xhr"

    node_modules/mappersmith/esm/index.mjs:2:20:
      2 │ import { XHR } from "./gateway/xhr";
        ╵                     ~~~~~~~~~~~~~~~

✘ [ERROR] Could not resolve "./gateway/http"

    node_modules/mappersmith/esm/index.mjs:3:21:
      3 │ import { HTTP } from "./gateway/http";
        ╵                      ~~~~~~~~~~~~~~~~

✘ [ERROR] Could not resolve "./gateway/fetch"

    node_modules/mappersmith/esm/index.mjs:4:22:
      4 │ import { Fetch } from "./gateway/fetch";
        ╵                       ~~~~~~~~~~~~~~~~~

✘ [ERROR] Could not resolve "./response"

    node_modules/mappersmith/esm/index.mjs:21:25:
      21 │ import { Response } from "./response";
         ╵                          ~~~~~~~~~~~~

To repeat the issue

src/index.ts

import { forge } from 'mappersmith'

console.log(forge)

esbuild.js

const { build } = require('esbuild')

build({
  entryPoints: ['./src/index.ts'],
  outdir: './dist',
  bundle: true,
  platform: 'node',
  format: 'esm',
  target: 'node16',
})

package.json

{
  "dependencies": {
    "esbuild": "^0.19.11",
    "mappersmith": "^2.43.1"
  }
}

And then run node esbuild.js

It seems to be that the issue happens because no .mjs extension are added in imports. I can temporarily patch it writing minimal plugin for esbuild, as described in this comment but perhaps there's a better way to fix it.

@klippx
Copy link
Collaborator

klippx commented Jan 8, 2024

Supported/documented use cases can be found in https://github.com/klippx/mappersmith-consumer/ and so far esbuild is not part of those. But it can be added, and should be added, especially if you can help me out.

I am familiar with the thread you posted. EvanW explained it well, and for me I have had to push through this issue before, but my setup is different. I use tsc + node, and I got around the issue by using this node loader https://github.com/barhun/extensionless#readme - but IDK if it works for your use case since esbuild writes the dist folder whereas I am used to tsc and following MS for better or worse.

I have not personally used esbuild, but it seems like an good candidate to add to the mappersmith consumer project. I bootstrapped something here that replicates your issue, and if you want to you can review / comment what is wrong. It doesn't seem to work with the plugin either as there is a number of issues in the dist folder produced:

  • only index.js is produced - the client.ts is not found by esbuild which means esbuild is confused when it comes to standard typescript import rules (we have to import using .js even in a TS file!)
  • it adds .mjs to EVERYTHING even if it ends with .js or is the single package entrypoint 'mappersmith'

But lets not go into the details of that, we can keep that discussion in the PR if you want to join the effort.

For this matter I would summarize is as follows:

  • I dont think there is a fix in mappersmith to be done here, we are using tsup to produce the package which is built on top of esbuild, which I believe is producing valid ESM and CJS artefacts
  • So the problem is likely be in the consumer project itself (i.e. your project, and also in the project of the PR I posted)
  • It should probably be solved using esbuild "magic"
  • Lets move the discussion to the PR for now until we resolve or reject it there. OK?

@klippx
Copy link
Collaborator

klippx commented Jan 8, 2024

I'm guessing the way people have been working around this in the meantime is to just not use the .mjs extension. The "type" field in package.json sets the meaning of the .js extension for your package. If you have "type": "module" in your package.json file, then .js and .mjs mean the same thing (and if you don't, .js and .cjs mean the same thing).

This is it, our built artect does not have the extension provided, and thus we need to use extensionless to help node find the file.

Unfortunately this doesn't work with the .mjs extension (I just tried it). The TypeScript type checker is unable to find ./index.ts if the import path is ./index.mjs.

Even if we did "fix" it we would be fighting typescript and might find ourselves in trouble with type imports etc. It might be that we wont find a good solution to the root cause and actually have relative imports that includes .mjs until MS fixes this in TS itself? 🤔

@manisenkov
Copy link
Author

Agree, the import should be fixed on the TS side, and actually producing workarounds does not feel like a sustainable solution in a long run.
Let's continue discussion on the linked PR 👍

@manisenkov
Copy link
Author

manisenkov commented Jan 8, 2024

Hmm, in fact even without esbuild (trying to just run a js file) I've got this


node:internal/errors:478
    ErrorCaptureStackTrace(err);
    ^

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/Users/manisenkov/mappersmith-test/node_modules/mappersmith/esm/mappersmith' imported from /Users/manisenkov/mappersmith-test/node_modules/mappersmith/esm/index.mjs
    at new NodeError (node:internal/errors:387:5)
    at finalizeResolution (node:internal/modules/esm/resolve:330:11)
    at moduleResolve (node:internal/modules/esm/resolve:907:10)
    at defaultResolve (node:internal/modules/esm/resolve:1115:11)
    at nextResolve (node:internal/modules/esm/loader:163:28)
    at ESMLoader.resolve (node:internal/modules/esm/loader:841:30)
    at ESMLoader.getModuleJob (node:internal/modules/esm/loader:424:18)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:76:40)
    at link (node:internal/modules/esm/module_job:75:36) {
  code: 'ERR_MODULE_NOT_FOUND'
}

package.json

{
  "volta": {
    "node": "16.20.1" // v19 gives me same results
  },
  "name": "mappersmith-test", 
  "dependencies": {
    "mappersmith": "^2.43.1"
  },
  "type": "module"
}

test.js

import forge from 'mappersmith'

console.log(forge)

What am I doing wrong here?

@klippx
Copy link
Collaborator

klippx commented Jan 9, 2024

I think that your expectations are not wrong and that this should work. The esm output does not have the .mjs extensions like it should. We are using tsup to generate esm, but it only works if you use extensionless or another workaround to convince node to run it.

I found related issues in tsup repo

I think I need to look into some hack to patch the dist/esm/*.mjs files to have the extension added to the output.

Ideally the built dist/esm folder should "just work" for your simple use case here without any third party magic to accommodate for the sloppy esm code that tsup has produced.

If you are interested in lending a hand or investigating you can take a look at tsup.config.ts

@klippx
Copy link
Collaborator

klippx commented Jan 9, 2024

I think I have fixed this now @manisenkov thanks a ton for valuable input and making this library better!

Fixed in 2.43.2

@klippx
Copy link
Collaborator

klippx commented Jan 9, 2024

Btw, I am not sure how things are working with node 16, we only have CI support for node 18 and node 20. It might be still some bad luck for you in node 16 land and I would recommend not using deprecated node versions.

@manisenkov
Copy link
Author

Thank you so much for fixing that, @klippx!
Just checked, it works like a charm with Node 16 as well 🎉

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.

2 participants