-
Notifications
You must be signed in to change notification settings - Fork 218
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
Export non-bundled module #232
Conversation
to make the flow with `npm link` and `npm install` from github work seamlessly
because "deep-fast-equal" exposes a CommonJS default export, which cannot be resolved when loading <script type="module"> in a browser
because the previous setup was inconsistently using "tsc" directly or via the run-script
explain the difference in README.md and add test
- replaced "jsdom" with "event-target-shim" - replaced "underscore" with "jsonpatch._areEquals" - replaced "jsonfile" with importing a ES module - use custom "jasmine-run.js" script, copied from https://github.com/Palindrom/JSON-Patch-Queue/blob/612caf0646bc4a5b3d5630d9ec37f8f2f83d2464/jasmine-run.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, tested with multiple Palindrom configurations
@tomalec, maybe you'd like to review my changes in this PR: b4b9de7...aa3e1db |
As `__esModule` flag is not a problem, lack of default export is a problem for many users that assumed it exists due to old typescript/webpack conventions This reverts commit 04671e5.
because it is also exported in "duplex"
because there are many directories and the function of "lib" was not clear
run `npm run build && npm run test` after merge Conflicts: .gitignore dist/fast-json-patch.js - ignored, to be rebuilt dist/fast-json-patch.min.js - ignored, to be rebuilt package.json src/core.ts - added `_areEquals` to default export test/jasmine.json test/spec/commonjs/requireSpec.js test/spec/webpack/importSpec.build.js - ignored, to be rebuilt test/spec/webpack/importSpec.src.js webpack.config.js - code formatted
to fix the merge
as it is now covered by index.js and index.mjs
@warpech Would you like to re-review it, as we pushed quite a few commits, since your last approval, or is it good to go? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at it so many times, I can't really see anything new here. Despite some errors in README.md. Would you mind fixing and merging this branch? I think it can be released as a prerelease.
README.md
Outdated
// document == { firstName: "Joachim", contactDetails: { phoneNumbers: [] }} | ||
```html | ||
<script type="module"> | ||
import { * as jsonpatch } from 'fast-json-patch/index.mjs'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to import * as jsonpatch from
README.md
Outdated
// { op: "replace", path: "/contactDetails/phoneNumbers/0/number", value: "123" }, | ||
// { op: "add", path: "/contactDetails/phoneNumbers/1", value: {number:"456"}} | ||
// ]; | ||
import { * as jsonpatch } from 'fast-json-patch/index.mjs'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
README.md
Outdated
// { op: "replace", path: "/contactDetails/phoneNumbers/0/number", value: "123" }, | ||
// { op: "add", path: "/contactDetails/phoneNumbers/1", value: {number:"456"}} | ||
// ]; | ||
import { * as jsonpatch } from 'fast-json-patch'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
as suggested at #232 (review)
No description provided.