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

fix: revert esm changes #197

Merged
merged 1 commit into from
Aug 13, 2021
Merged

fix: revert esm changes #197

merged 1 commit into from
Aug 13, 2021

Conversation

Nebulis
Copy link
Contributor

@Nebulis Nebulis commented Aug 12, 2021

closes #195

This PR reverts all the changes to make OA an ESM-only library. The reasons for the revert are detailed below.

Disclaimer

Once OA v6.0.0 was released, I tried to use it in another library. I faced many problems, specifically with Jest, and decided to dig more and check whether there were issues with other tools.

I noticed that the library built didn't work directly (without specific configuration) with the following tools:

  • node
  • ts-node
  • jest
  • ts-jest
  • webpack

Node

Path

When trying to use the library in a node project, the following error happened:

Directory import '/home/nebulis/IdeaProjects/test-esm/node_modules/@govtechsg/open-attestation/dist/shared/validate' is not supported resolving ES modules imported from /home/nebulis/IdeaProjects/test-esm/node_modules/@govtechsg/open-attestation/dist/index.js

The reason is simple: we didn't use full path to import files. We relied on common js behavior, which doesn't require the extension (.js) and which automatically loads index files if it exists.

To fix there is a node flag:

❯ node --experimental-specifier-resolution=node src/index.js

CommonJS

Once the path is fixed for node, a new error happened:

SyntaxError: Named export 'identity' not found. The requested module 'lodash' is a CommonJS module, which may not support all module.exports as named exports.

There is no way to fix this with a flag. We must fix our library and changed all imports that are from CJS libraries. To manage to test our library with the rest of the tools, I fixed manually, inside the node_modules folder. Once fixed, the library was usable with node.

Additional readings:

Jest

To have Jest working, the first thing is to follow the documentation:

  • add transform {} (useless)
  • add NODE_OPTIONS=--experimental-vm-modules

And additionally we need a custom resolver (answer found somewhere tin the Jest issues:

  • add custom resolver => "resolver": "./export_maps_resolver.cjs"
// export_maps_resolver.cjs
const resolver = require("enhanced-resolve").create.sync({
  conditionNames: ["require", "node", "default", "import"],
  extensions: [".js", ".json", ".node", ".ts"],
});

module.exports = function (request, options) {
  if (request === "crypto") {
    return "crypto";
  }
  return resolver(options.basedir, request);
};

Additional readings:

ts-kest

To have ts-jest working, the first thing is to follow the documentation, in addition to the steps used for Jest above:

{
  // jest config
  "preset": "ts-jest",
  "testEnvironment": "node",
  "extensionsToTreatAsEsm": [".ts"],
  "globals": {
    "ts-jest": {
      "useESM": true
    }
  }
}

ts-node

When using ts-node, the following error happened:

Unknown file extension ".ts"

ts-node doesn't support ESM for the moment. The progress can be followed in TypeStrong/ts-node#1007. There is an experimental loader that can be used by following the direction in the issue:

  • add "module": "ESNext" into tsconfig.json
  • add "type": "module" into package.json
  • npm install ts-node
  • node --experimental-specifier-resolution=node --loader ts-node/esm src/index.js

It's important to use ts-node as a loader.

Webpack

Webpack didn't work for the same reason as node. The imports are not valid. There is a flag to set, to force webpack to infer the path:

// webpack.config.js
{
  test: /\.m?js/,
  resolve: {
    fullySpecified: false
  }
}

Additional reading:

Issues

Overall there are 2 issues

  1. Our import paths are wrong. All our paths should directly target the files, without any inference required.
  2. We must import differently CJS modules:
  • lodash
  • hs-sha3
  • flatley

Path

We could expect typescript to help us, but no. Like for custom paths, maintainers decided that it's not their job. So we are left alone. You can find a discussion on the topic below:

There are tools to help:

I tried to find a lint rule to help, but it looks like ESM and Typescript don't move together in the same direction:

If you read carefully the discussion, you will find out that you can import .js files, inside typescript files, even if the file you import is a typescript file (and not a javascript file). Heresy? :)

This solution works, however, I faced issues with ts-jest. Some people made it work, but at this point, I was over with this, and decided to not dig more

CJS modules

The fix is quite simple, but I tried to find a way to trigger an error on build or on lint. Unsuccessfully.

Conclusion

With all this information, we decided to revert back to our old process. ESM and TS don't play well together and we may reconsider in the future if there are improvements with the tooling. The goal must be to help library users and developers but right now, it will just add a burden for both.

"build:cjs": "tsc --module commonjs --outDir dist/cjs --project ./tsconfig.prod.json",
"build:esm": "tsc --module es2015 --outDir dist/esm --project ./tsconfig.prod.json",
"build:type": "tsc -d --emitDeclarationOnly --outDir dist/types --project ./tsconfig.prod.json",
"build:umd": "rollup -c",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since rollup is building from ./dist/esm/index.js, should we do

"build:umd": "npm run build:cjs && rollup -c",

Copy link
Contributor

@eugbyte eugbyte Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to update the circle ci to install rollup

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hum no I will keep it as it was before

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for rollup I forgot to put it back. Fixed

@john-dot-oa
Copy link

🎉 This PR is included in version 6.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: ESM imports not working
3 participants