Closed shockey closed 5 years ago
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
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.
To me it seems like a weird behavior of typescript compiler https://github.com/Microsoft/TypeScript/issues/14351
It sets _esModule
if our source is using any import/export
regardless of the output module CJS format
Issue was introduced by updating typescript at https://github.com/Starcounter-Jack/JSON-Patch/commit/54911b2ae317a46d1f3450ccd10ced298b820ea3#diff-529047924cdb0e5562a8de34628a855dR111
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.
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 https://github.com/Starcounter-Jack/JSON-Patch/pull/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.
fast-json-patch@2.2.0
is breaking builds in Swagger Client (see https://github.com/swagger-api/swagger-js/issues/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: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:...Babel translates my import to:
...which means at runtime, my code will see
__esModule: true
infast-json-patch
and therefore not wrap the exported result from it in adefault
key: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], ... } }
.