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: fixed issues with ESM conversion not working #223

Merged
merged 3 commits into from
Apr 25, 2021
Merged

Conversation

spiltcoffee
Copy link
Owner

@spiltcoffee spiltcoffee commented Apr 24, 2021

What issues is this pull request related to?

#222

What changes were made in this pull request?

Difficult problem to solve since both TypeScript and Jest had specific issues that needed to be
addressed.

Node firstly required that relative files resolve exactly, so now using the "exports" key to achieve this.

Then TypeScript was properly reading this key, but wasn't able to figure out where the .d.ts files were, so now using the "typesVersions" key to achieve this.

Then Jest didn't knowhow to read the "exports" key and generally couldn't test the project anymore, so now using the "enhanced-resolve" resolver to try and resolve files with first (has proper support for ESM, it would appear).

Lastly, changed coverage to look at .js files instead of .ts files, so the codecov report is going to look strange here.

@spiltcoffee
Copy link
Owner Author

spiltcoffee commented Apr 24, 2021

I believe this PR accidentally fixes #221 by using enhanced-resolve instead of jest's default resolver which has timing issues. nevermind, the bug still exists!

Difficult problem to solve since both TypeScript and Jest had specific issues that needed to be
addressed. Node firstly required that relative files resolve exactly, so now using the "exports" key
to achieve this. Then TypeScript was properly reading this key, but wasn't able to figure out where
the .d.ts files were, so now using the "typesVersions" key to achieve this. Then Jest didn't know
how to read the "exports" key and generally couldn't test the project anymore, so now using the
"enhanced-resolve" resolver to try and resolve files with first (has proper support for ESM, it
would appear). Lastly, changed coverage to look at .js files instead of .ts files, so the codecov
report is going to look strange here.

fix #222
@spiltcoffee spiltcoffee force-pushed the esm-tsconfig branch 5 times, most recently from 51c8291 to 20230d9 Compare April 24, 2021 14:36
@spiltcoffee
Copy link
Owner Author

May need to investigate this: https://github.com/SitePen/remap-istanbul

@codecov
Copy link

codecov bot commented Apr 24, 2021

Codecov Report

Merging #223 (5305b9a) into main (c83aa08) will decrease coverage by 1.56%.
The diff coverage is 94.89%.

Impacted file tree graph

@@             Coverage Diff             @@
##              main     #223      +/-   ##
===========================================
- Coverage   100.00%   98.43%   -1.57%     
===========================================
  Files           33       34       +1     
  Lines          309      446     +137     
  Branches        56       71      +15     
===========================================
+ Hits           309      439     +130     
- Misses           0        1       +1     
- Partials         0        6       +6     
Impacted Files Coverage Δ
packages/postdfm/src/index.ts 100.00% <ø> (ø)
packages/@postdfm/dfm2ast/src/grammar.ts 94.89% <94.89%> (ø)

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 1049405...5305b9a. Read the comment docs.

@spiltcoffee spiltcoffee force-pushed the esm-tsconfig branch 6 times, most recently from a6fc29d to 686daa5 Compare April 25, 2021 03:20
@spiltcoffee spiltcoffee merged commit f85cae4 into main Apr 25, 2021
@spiltcoffee spiltcoffee deleted the esm-tsconfig branch April 25, 2021 03:30
@spiltcoffee
Copy link
Owner Author

Will reduce code coverage, but only because of more accurate information.

@spiltcoffee
Copy link
Owner Author

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.

1 participant