tulios / mappersmith

is a lightweight rest client for node.js and the browser
https://www.npmjs.com/package/mappersmith
MIT License
335 stars 72 forks source link

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

Closed manisenkov closed 10 months ago

manisenkov commented 10 months ago

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 commented 10 months ago

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:

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:

klippx commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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