[chore]: Standardize package exports in a way that silences pnpm warnings #1590
Conversation
|
Note, I ran into issues in multiple PRs that ts-node was not able to use "import" statements because the scripts code was not a module. "tsx" is already installed and is the more modern way to execute a TypeScript script, so I modified the format and lint scripts to use it. |
|
Looks like jest uses ts-node. Going to look into fixes. |
|
IIRC we have explicitly not done this before because it's actually correct and pnpm is wrong to be sending warnings. The ordering of |
|
If that's the case, why are our orerings not consistent throughout the codebase? Do you have an example of a toolchain which has broken which I can test? |
|
I don't remember for sure, I just remember trying to fix this same thing a few months back and going down a rabbit hole where I decided not touching it was the better course...maybe it's fine? I just know that this stuff has been very fragile to mess with previously. |
yeah, very fragile... I wouldn't trust our tests to cover these things. |
|
Trying to do some research because I agree this can both be fragile and important. The node docs make it clear that default should be last but only give guidance that you should go “more to less” specific with import vs require (wtf does that even mean?!) I checked the TypeScript docs and it definitely says “types” must come first. I ran across a bug that noted it works anyway and the team openly considered a breaking change to prevent the proliferation of malformed packages. The best I can guess from this is that ESM modules should put import first and CSM modules should put require first because that is the naive syntax for the package, but then why would pnpm be giving us warnings since we’re creating CSM modules? fwiw ChatGPT hallucinates a recommendation for the order here. I’ll add links after my dr apt |
|
There’s an in progress Node docs repo that goes into some of the pitfalls. FWIW, both of its examples have import before require. It also raises another question: given our very high minimum node version, why are we producing CSM instead of the more performant ESM? |
"high minimum node version" happened organically as we discovered that some things weren't working... and I think originally we just started by copy-pasting the boilerplate from some firebase SDK, and tweaked things until it worked for us... then tweaked a lot more things as we noticed things breaking in various environments. |
|
After more research, I’m starting to think that we should not only make this change, but also change from CSM to ESM (which is a breaking change, so no time like the present!) ESM is supported in all LTS node versions, loads faster (async I/O), supports tree shaking for smaller deployment sizes, and can be more easily shared in browsers (I know that’s not supported yet, but I’d like to get there). This would require zero code changes on our side because TS syntax is already ESM. I also don’t think it requires any customers to change code because we’re actually creating UMD with our exports annotations. My somewhat informed assumption is that the change in module type would change what typescript transpiles to. I can verify if you want (once I get my test app working for no-skipLibCheck PR The only downside is ESM is that exports are no longer dynamic and therefore you can’t replace them with mocks like Sinon. |
|
We can't switch to ESM because we are a library and we want maximally
compatible code. Even if people are using a version of Node.js that
supports ESM having a default ESM package is very hard to use from CJS and
lots of people (e.g. Next.js projects) are still CJS packages.
We are already shipping both CJS and ESM versions, and ESM imports will use
our ESM code.
…On Wed, Jan 8, 2025, 2:20 PM Thomas Bouldin ***@***.***> wrote:
After more research, I’m starting to think that we should not only make
this change, but also change from CSM to ESM (which is a breaking change,
so no time like the present!)
ESM is supported in all LTS node versions, loads faster (async I/O),
supports tree shaking for smaller deployment sizes, and can be more easily
shared in browsers (I know that’s not supported yet, but I’d like to get
there). This would require zero code changes on our side because TS syntax
is already ESM. I also don’t think it requires any customers to change code
because we’re actually creating UMD with our exports annotations.
My somewhat informed assumption is that the change in module type would
change what typescript transpiles to. I can verify if you want (once I get
my test app working for no-skipLibCheck PR
The only downside is ESM is that exports are no longer dynamic and
therefore you can’t replace them with mocks like Simon.
thoughts @mbleigh <https://github.com/mbleigh> @pavelgj
<https://github.com/pavelgj> ?
—
Reply to this email directly, view it on GitHub
<#1590 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAAH7RPIMHAB4BPAGB4XLL2JWQEJAVCNFSM6AAAAABUYUMFYOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNZYG44DENJWHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Fixes #1589 to clean up warnings about our package ordering (which is inconsistent within the repository). Thank you ChatGPT for the suggested ordering, which silences pnpm. I worry but have not verified that the old way might confuse node or typescript on import style.
Checklist (if applicable):