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

jest-diff: Export as ECMAScript module #8873

Merged
merged 7 commits into from
Aug 26, 2019

Conversation

pedrottimark
Copy link
Contributor

@pedrottimark pedrottimark commented Aug 24, 2019

Summary

Because a caller might need to construct Diff instances to write a custom cleanup algorithm, it must be exported as a class value instead of a type.

Breaking change: Because we rewrote as ECMAScript and TypeScript exports, then the class value can be used in type declarations! Thank you, Simen for suggesting this solution ❤️

CommonJS callers rewrite default export:

const diff = require('jest-diff'); // Jest 24 or earlier
const diff = require('jest-diff').default; // Jest 25 or later

But see README.md because new named export diffStringsUnified might be better for you

However, if I understand the situation correctly, for TypeScript declarations like diff: Diff or diffs: Array<Diff> when the class is imported across a package boundary, an additional type declaration is needed to avoid error TS2749: 'Diff' refers to a value, but is being used as a type here.

Corrected and edited README.md

Did not add to CHANGELOG.md because it is an immediate Began as correction to #8841

Test plan

  • Added diffStringsRaw.test.ts which verifies exports, including Diff as type
  • The use in jest-matcher-utils also verifies exports, including Diff as type

@codecov-io
Copy link

codecov-io commented Aug 24, 2019

Codecov Report

Merging #8873 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8873      +/-   ##
==========================================
- Coverage   64.12%   64.11%   -0.01%     
==========================================
  Files         275      275              
  Lines       11629    11623       -6     
  Branches     2845     2845              
==========================================
- Hits         7457     7452       -5     
  Misses       3545     3545              
+ Partials      627      626       -1
Impacted Files Coverage Δ
packages/jest-diff/src/index.ts 78.04% <ø> (-2.39%) ⬇️
packages/jest-snapshot/src/print.ts 70.37% <100%> (ø) ⬆️
packages/jest-matcher-utils/src/index.ts 82.35% <100%> (-0.12%) ⬇️
packages/jest-diff/src/printDiffs.ts 95% <0%> (+1.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e6a8e8...056ecc7. Read the comment docs.

packages/jest-diff/src/index.ts Outdated Show resolved Hide resolved
packages/jest-diff/src/index.ts Show resolved Hide resolved
packages/jest-diff/README.md Outdated Show resolved Hide resolved
@pedrottimark pedrottimark changed the title jest-diff: Export Diff constructor as value instead of type jest-diff: Rewrite as ECMAScript module Aug 24, 2019
@SimenB SimenB requested review from jeysal and thymikee August 24, 2019 18:52
@pedrottimark
Copy link
Contributor Author

Do y’all think it would help or hurt to distinguish versions for breaking change in README:

  • import diffLinesUnified from 'jest-diff'; in ECMAScript modules
  • const diffLinesUnified = require('jest-diff').default; in CommonJS modules (jest-diff version 25 or later)
  • const diffLinesUnified = require('jest-diff'); in CommonJS modules (jest-diff version 24 or earlier)

And also move ECMAScript example above CommonJS example?

And identify version of new options and named exports?

@SimenB
Copy link
Member

SimenB commented Aug 24, 2019

The readme won't be included for jest 24, so I don't think that's needed.

packages/jest-diff/src/index.ts Outdated Show resolved Hide resolved
@pedrottimark pedrottimark changed the title jest-diff: Rewrite as ECMAScript module jest-diff: Export as ECMAScript module Aug 26, 2019
@pedrottimark pedrottimark merged commit fb7b132 into jestjs:master Aug 26, 2019
@pedrottimark pedrottimark deleted the export-Diff branch August 26, 2019 15:04
@chrisblossom
Copy link
Contributor

Wouldn't this be better as a named export instead of default?

const diff = require('jest-diff'); // Jest 24 or earlier
const { diff } = require('jest-diff'); // Jest 25 or later

@SimenB
Copy link
Member

SimenB commented Sep 5, 2019

Either works for me, I'll leave it to @pedrottimark 🙂 It does make it cleaner for CJS, though

@chrisblossom
Copy link
Contributor

I bring this up because it probably should be standardized across all of jest’s packages. My vote goes to named exports for whatever its worth.

@pedrottimark
Copy link
Contributor Author

Thank you for bringing this up.

Among others, Nicholas C. Zakas has explained benefits of named versus default export.

The only reason why it might not happen before Jest 25 is that I am right now exploring how the public API might grow so a naming convention for the exports will be consistent.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants