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

Monorepo: Set "type": "module" in package.json files (default ESM internally) #3494

Merged
merged 14 commits into from
Jul 11, 2024

Conversation

holgerd77
Copy link
Member

First round test run to see what (if something) breaks if we switch over.

Depending on results of first commit (Util) we can expand.

Not sure if I'll finish this, this can be happily picked up! 🙂

…works, lint works, docs do not work (maybe unrelated), examples work)
Copy link

codecov bot commented Jul 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 11.07%. Comparing base (d24ca11) to head (ed2e16f).
Report is 47 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (d24ca11) and HEAD (ed2e16f). Click for more details.

HEAD has 4 uploads less than BASE
Flag BASE (d24ca11) HEAD (ed2e16f)
common 1 0
blockchain 1 0
util 1 0
statemanager 1 0
Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block 72.71% <ø> (?)
blockchain ?
common ?
genesis 0.00% <ø> (?)
statemanager ?
util ?

Flags with carried forward coverage won't be shown. Click here to find out more.

@holgerd77
Copy link
Member Author

Ok. There was actually so much (all? not sure about doc generation, but that seems minor, also not sure about codecov/project) working (otherwise full CI passing), that I will directly test will all libraries switched over.

@holgerd77
Copy link
Member Author

Ah. Finally at least something is breaking. 🙏 😂

… module causes too much problem in this intense usage setup (not ESM ready)
@holgerd77
Copy link
Member Author

I battled with the trie examples quite some time, where the debug import from the view.ts utility debug module caused problems in the logDemo.ts and walkDemo.ts files, so where the debug functionality was integrated and so heavily extended.

I finally came to the conclusion that this is not worth to fix for this very specialized debug/log functionality, mostly meant to be used for internal purposes, and moved the both the view.ts file out of src and the respective demo as well to a new scripts folder. I very much had the impression that this was some weird misbehavior along tooling, since debug is generally an ESM "problem child" still - see debug-js/debug#786.

Additionally - after having a look - I felt that this was generally not really to justify that we put this view.ts module into production code just to have easy direct compilation and deep import access from the dist folder. Functionality should still work though, or at least easy to re-vitalize if desired at some point.

Copy link
Contributor

@acolytec3 acolytec3 left a comment

Choose a reason for hiding this comment

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

LGTM

@holgerd77 holgerd77 merged commit 7ec47a3 into master Jul 11, 2024
35 of 36 checks passed
@holgerd77 holgerd77 deleted the monorepo-type-module-the-final-switch branch July 11, 2024 19:12
@holgerd77 holgerd77 restored the monorepo-type-module-the-final-switch branch July 12, 2024 08:55
@holgerd77 holgerd77 deleted the monorepo-type-module-the-final-switch branch October 7, 2024 09:24
@holgerd77
Copy link
Member Author

@acolytec3 Just writing release notes and unsure about this: would you agree that this change has no impact on library users (since we have this pointing to ESM/CJS and there the type property from package.json is updated anyhow)? Or is this missing out some factors? 🤔

@acolytec3
Copy link
Contributor

To my understanding, yes, this should be correct. NPM always looks to the nearest package.json (I believe starting in the ./dist folder) for a package and then working it's way down from there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants