digitalbazaar / cborld

A Javascript CBOR-LD processor for web browsers and Node.js apps.
https://json-ld.github.io/cbor-ld-spec/
BSD 3-Clause "New" or "Revised" License
16 stars 12 forks source link

Bundle with Babel, remove esm module loader #49

Closed zhenwenc closed 2 years ago

zhenwenc commented 3 years ago

Description

Remove:

Changed:

Motivation

Jest seems to have compatibility issues with modules generated by the esm module loader, which causes the cborld library to produce unexpected results, and very high memory and CPU consumption while running each test case, as well as rapidly slowing down the execution (it happened to take almost 20 seconds to run a simple test case).


To illustrate the problem, I have created a small repro repo with my assumptions for the root cause: https://github.com/zhenwenc/cbor-issue/blob/main/src/__tests__/cborld-encode.spec.ts

You can view the failure test result in this pipeline: https://github.com/zhenwenc/cbor-issue/runs/2780071449?check_suite_focus=true

My suspicion is that the subclass instances of CborldEncoder aren't been encoded by the defined encode function due to the instanceof check returns incorrect result. For example:

 const encoder = ContextEncoder.createEncoder({ value, transformer: this });
 expect(encoder instanceof CborldEncoder).toBe(true); // but returns false

Therefore, the encoder token above will be unexpectedly processed by the cborg default encoders.

This is likely related to the compatibility issue of CJS generation from ESM with Jest which seems to have its own require implementation.


Therefore, I suggest would you consider transpiling the source code with a bundler before publishing the package? This PR contains the minimal configuration for Babel.

Thanks!

dlongley commented 3 years ago

@davidlehn, @dmitrizagidulin -- can you take a look? While we're supportive of addressing the issue, one thing we really don't want to do is make it so that development (including symlinking in other projects) requires adding a compilation step to dev cycles.

dlongley commented 3 years ago

I should say: First, thank you for creating a repro repo for this! We really appreciate it.

Second, I cloned cbor-issue and ran the tests (via npm test) without issue (they executed quickly and all passed). I also ran:

./node_modules/.bin/jest --forceExit cborld-encode.spec.ts

Individually to see what happened -- it also passed and output a lot of diagnostic information. So, something else is going on here.

@davidlehn -- can you give this a shot?

dlongley commented 3 years ago

I was running node v14.16.0. Looks like the test in the link above ran v12.22.1 -- so I tried that as well and the tests still pass for me without issue.

dlongley commented 3 years ago

Hmm, I'm guessing this commit was intended to make the tests pass by running the module through babel. The tests do get slow and fail if I roll back that commit.

zhenwenc commented 3 years ago

Hmm, I'm guessing this commit was intended to make the tests pass by running the module through babel. The tests do get slow and fail if I roll back that commit.

@dlongley Thank you very much for looking into the issue! You're correct, this commit was trying to confirm the issue is caused by esm as well as seeking potential workarounds.

Sorry for the confusion, I should've mentioned this earlier. I will move this commit to a separate branch.

zhenwenc commented 3 years ago

Just a friendly reminder @dlongley , is there anything else you think it's missing in this PR? We would keen to have this fix merged. Many thanks!

dlongley commented 2 years ago

Can this be closed now the ESM fixes have landed?

zhenwenc commented 2 years ago

@dlongley Since the library had migrated to use native ESM, this solution will not be suitable anymore. Closing this PR.

Besides, considering native ESM is still pretty young, library consumers may not be able to catch up with the upgrades. From my understanding, ESM projects are able to use CJS dependencies, but not the other way around. Would you consider shipping CJS format bundles?

davidlehn commented 2 years ago

@zhenwenc Yeah, an unfortunate side effect of the one-way(ish) nature of CJS/ESM loading is that once a CJS package has any ESM-only dependencies, it has to convert over too. Which will likely push all JS to ESM over time. Good or bad, that's how it is. CJS can use ESM via async import() calls, but that's not always possible, or easy, to do. With our libraries we are trying to keep things ESM-only from now on, with rare exceptions if needed. Having both ESM and built CJS adds complexity for both packaging and testing. And it becomes difficult if we want to scale that process to the many many packages we maintain. We're leaning towards simplicity where possible.

Are you having difficulties with tooling not handling ESM, or your application? I think most recent tooling can handle ESM. Converting large projects and libraries can be a hassle, which I've recently found out, but if you do a top-down conversion, the ESM parts can still load CJS.

zhenwenc commented 2 years ago

@davidlehn Thank you for the detailed reply! Much appreciated!

Yeah, agree that moving towards to native ESM is a wise choice that reduces the hassle of mixing different CJS and ESM. The applications and libraries I am working on haven't migrated to ESM yet, and it will require quite some effort for the transition. Thanks a lot for the advice on the top-down conversion strategy. 👍