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

[email protected] is incorrectly presenting as a transpiled ES Module #233

Closed
shockey opened this issue Aug 2, 2019 · 6 comments · Fixed by #236
Closed

[email protected] is incorrectly presenting as a transpiled ES Module #233

shockey opened this issue Aug 2, 2019 · 6 comments · Fixed by #236

Comments

@shockey
Copy link

shockey commented Aug 2, 2019

[email protected] is breaking builds in Swagger Client (see swagger-api/swagger-js#1460).

Current behavior

The problem is that fast-json-patch is combining the __esModule hint with an exports object that follows the CJS exports pattern.

2.2.0 is setting an non-enumerable __esModule hint property that wasn't present before:

➜ node -e 'const fjp = require("fast-json-patch"); const util = require("util"); console.log(util.inspect(fjp, { showHidden: true, depth: 0 }));'
{ [__esModule]: true,
  applyOperation: [Object],
  applyPatch: [Object],
  applyReducer: [Object],
  getValueByPointer: [Object],
  validate: [Object],
  validator: [Object],
  JsonPatchError: [Object],
  deepClone: [Object],
  escapePathComponent: [Object],
  unescapePathComponent: [Object],
  unobserve: [Object],
  observe: [Object],
  generate: [Object],
  compare: [Object] }

This property is used in transpilers and module loaders to handle interoperability between CJS modules and ESM modules represented as JavaScript objects -- specifically, CJS modules need to be wrapped inside another object's default property. More info here.

All of this means that when I use import syntax to use fast-json-patch in my Babel-enabled project:

import fastJsonPatch from "fast-json-patch"

fastJsonPatch.applyOperation()

...Babel translates my import to:

var _fastJsonPatch = _interopRequireDefault(require("fast-json-patch"));

function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { "default": obj }; }

_fastJsonPatch["default"].applyPatch();

...which means at runtime, my code will see __esModule: true in fast-json-patch and therefore not wrap the exported result from it in a default key:

➜ node -e 'var _fastJsonPatch = _interopRequireDefault(require("fast-json-patch")); function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { "default": obj }; }; _fastJsonPatch["default"].applyPatch();'
[eval]:5
  _fastJsonPatch["default"].applyPatch();
                          ^

TypeError: Cannot read property 'applyPatch' of undefined
    at [eval]:5:27
    at ContextifyScript.Script.runInThisContext (vm.js:50:33)
    at Object.runInThisContext (vm.js:139:38)
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (module.js:652:30)
    at evalScript (bootstrap_node.js:466:27)
    at startup (bootstrap_node.js:167:9)
    at bootstrap_node.js:612:3

Expected behavior

fast-json-patch should either be not setting __esModule (which was the previous behavior), or should be exporting an object that matches ESM syntax, e.g. { __esModule: true, default: { applyOperation: [Object], ... } }.

@warpech
Copy link
Collaborator

warpech commented Aug 5, 2019

Thanks for a detailed bug report.

Could you please check how does the branch https://github.com/Starcounter-Jack/JSON-Patch/tree/use-ES6-modules work for you?

To install a GitHub branch as NPM package:

npm install git://github.com/Starcounter-Jack/JSON-Patch.git#use-ES6-modules

Or add this to package.json:

"fast-json-patch": "github:Starcounter-Jack/JSON-Patch#use-ES6-modules"

And then follow the instruction in README.md to import:

https://github.com/Starcounter-Jack/JSON-Patch/blob/use-ES6-modules/README.md#adding-to-your-project

@shockey
Copy link
Author

shockey commented Aug 5, 2019

Hi @warpech, this branch is causing SyntaxErrors in our tests:

 FAIL  test/specmap/properties.js
  ● Test suite failed to run

    Jest encountered an unexpected token

    This usually means that you are trying to import a file which Jest cannot parse, e.g. it's not plain JavaScript.

    By default, if Jest sees a Babel config, it will use that to transform your files, ignoring "node_modules".

    Here's what you can do:
     • To have some of your "node_modules" files transformed, you can specify a custom "transformIgnorePatterns" in your config.
     • If you need a custom transformation specify a "transform" option in your config.
     • If you simply want to mock your non-JS modules (e.g. binary assets) you can stub them out with the "moduleNameMapper" config option.

    You'll find more details and examples of these config options in the docs:
    https://jestjs.io/docs/en/configuration.html

    Details:

    /Users/kyle/Code/~swagger/js/node_modules/fast-json-patch/module/duplex.js:6
    import { _deepClone, _objectKeys, escapePathComponent, hasOwnProperty } from './helpers.js';
    ^^^^^^

I see that in your use-ES6-modules branch, your package.json's main value points to a file (module/duplex.js) that uses import/export syntax. This isn't a best practice, since most environments (browsers, anything before Node 12) can't handle that syntax. If you want to start shipping ESM syntax, using module to point to files with ESM syntax is the generally-accepted practice -- this Stack Overflow answer is a good touchpoint for this.


Stepping back a bit... in my view, the problem isn't with the module syntax (import/export vs require()) that you're using internally or shipping out to consumers, since the ecosystem isn't really ready for raw import/export syntax it's best to keep using require() for now.

The root of the issue is the combination of (a) the shape of object that is exported from the module and (b) the __esModule hint provided to consumers in the module.

@tomalec
Copy link
Collaborator

tomalec commented Aug 7, 2019

To me it seems like a weird behavior of typescript compiler microsoft/TypeScript#14351
It sets _esModule if our source is using any import/export regardless of the output module CJS format

@tomalec
Copy link
Collaborator

tomalec commented Aug 7, 2019

Issue was introduced by updating typescript at 54911b2#diff-529047924cdb0e5562a8de34628a855dR111

tomalec added a commit that referenced this issue Aug 8, 2019
the version that was not adding `Object.defineProperty(exports, "__esModule", { value: true });`
Therefore, resulting in module transpilable to one with unintended default export.

Fixes backward compatibility and #233
@tomalec
Copy link
Collaborator

tomalec commented Aug 8, 2019

It occurred to be related to backward incompatible change related to undocumented default export.
Swagger and many other projects started to use fast-json-patch as it would have default export, that has a map of all named exports (import jsonPatch from 'fast-json-patch'; jsonPatch.apply(...)).
We don't think it's a desired usage, as that is what import * is for.
Probably, it was only made possible due to bug in typescript, that was fixed, and fixing TS introduced original issue :)

We will revert this breaking change, and release a new version to support Swagger among many other projects that are currently using default export as a map of all named ones.

However in README we would still recommend using named imports.

warpech added a commit that referenced this issue Aug 9, 2019
@warpech
Copy link
Collaborator

warpech commented Aug 9, 2019

Thank you @shockey for your help in resolving this. We have fixed this regression in a patch release 2.2.1 and added exernal API usage tests for this to never fail again (PR #236)

Anyway we will keep recommending using named imports in README.md, because they are less error prone. See https://humanwhocodes.com/blog/2019/01/stop-using-default-exports-javascript-module/ for a nice list of reasons.

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