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

build esm files to separate folder #684

Merged
merged 1 commit into from Apr 6, 2022
Merged

Conversation

nihgwu
Copy link
Contributor

@nihgwu nihgwu commented Mar 27, 2022

I build a package based on sucrase and the current folder structure has raised lots of problem, I've tried my best to workaround them without touching sucrase, but now I think it's impossible.

The current folder structure doesn't work for ESM as mjs extension is required by Node for importing an ESM file, while sucrase doesn't, so for webpack5 we have to add resolve.fullySpecified: false as a workaround, and for esbuild probably we need other workaround evanw/esbuild#2128 (comment), but for some build tools they don't expose the way to config esbuild, like Remix, Vite supports partially and doesn't change that config, so only alias to cjs version works for Vite, but there is no way to do that in Remix

So the case is that, even we import sucrase/dist/index.mjs, esbuild will import NameManager.js instead of NameManager.mjs, while webpack will import NameManager.mjs even we import from sucrase/dist/index.js

After this change, we don't need any workarounds to make it work properly in the frontend world, and it won't break the Node environment as the current mjs file importing is wrong and doesn't work

I didn't move cjs files to cjs folder on purpose, in case some one would import the sub files manually

@nihgwu
Copy link
Contributor Author

nihgwu commented Mar 31, 2022

putting esm to dist will break getVersion, probably we should simply build esm to root folder

@nihgwu
Copy link
Contributor Author

nihgwu commented Apr 4, 2022

@alangpierce WDYT? I've published my change as sucrase-esm and it solve all the problems I encountered https://github.com/nihgwu/react-runner/pull/110/files

@github-actions
Copy link

github-actions bot commented Apr 6, 2022

Benchmark results

Before this PR: 334.2 thousand lines per second
After this PR: 332.5 thousand lines per second

Measured change: 0.51% slower (0.66% slower to 0.76% faster)
Summary: Likely no significant difference

@codecov
Copy link

codecov bot commented Apr 6, 2022

Codecov Report

Merging #684 (d49340c) into main (57b8eff) will increase coverage by 0.07%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #684      +/-   ##
==========================================
+ Coverage   82.59%   82.67%   +0.07%     
==========================================
  Files          56       56              
  Lines        5694     5932     +238     
  Branches     1343     1343              
==========================================
+ Hits         4703     4904     +201     
- Misses        708      745      +37     
  Partials      283      283              
Impacted Files Coverage Δ
src/index.ts 90.19% <0.00%> (ø)
src/util/formatTokens.ts 70.58% <0.00%> (-10.06%) ⬇️
src/parser/util/identifier.ts 90.90% <0.00%> (-4.10%) ⬇️
src/util/shouldElideDefaultExport.ts 77.77% <0.00%> (-3.48%) ⬇️
src/util/getClassInfo.ts 87.85% <0.00%> (-1.85%) ⬇️
src/parser/plugins/jsx/index.ts 91.33% <0.00%> (-1.48%) ⬇️
src/identifyShadowedGlobals.ts 88.09% <0.00%> (-1.10%) ⬇️
src/parser/plugins/flow.ts 63.99% <0.00%> (-1.09%) ⬇️
src/parser/traverser/expression.ts 88.36% <0.00%> (-0.13%) ⬇️
src/Options.ts 100.00% <0.00%> (ø)
... and 20 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

Copy link
Owner

@alangpierce alangpierce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @nihgwu , thanks for the PR! And sorry for the delay, I've had my hands full with other life things, but trying to get back to open source work now.

As you can tell, the current ESM structure was added when the best practices were all a bit speculative, so happy to restructure it in a way that works better. I was a little worried about people relying on the .mjs or .d.ts files in the old directory structure, but that seems pretty unlikely, and even so, it's not part of the public interface for the library, so it should be fine to change without a semver-major bump.

Changing getVersion to a hard-coded string sounds good. I'll probably drop that function in a 4.0 release at some point.

There's a test failure that I already fixed on the main branch, so I'll merge anyway and cut a release.

@alangpierce alangpierce merged commit 495ebdc into alangpierce:main Apr 6, 2022
@alangpierce
Copy link
Owner

Just published this as 3.21.0

@jpdriver
Copy link

jpdriver commented Apr 6, 2022

thanks @alangpierce 👏🏻

@nihgwu
Copy link
Contributor Author

nihgwu commented Apr 6, 2022

@alangpierce Thank you so much, I'll give it a shot right away

UPDATE: It works like a charm, thank you again for your great work, I love your package so much, that's just what I dreamed for a modern builder

@nihgwu nihgwu deleted the neo/esm branch April 6, 2022 21:05
1Lighty pushed a commit to Astra-mod/sucrase that referenced this pull request Aug 14, 2022
Co-authored-by: Neo Nie <gongwu.nie@revolut.com>
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 this pull request may close these issues.

None yet

3 participants