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: make config package cjs-only #755

Merged
merged 3 commits into from
May 23, 2023
Merged

fix: make config package cjs-only #755

merged 3 commits into from
May 23, 2023

Conversation

rflechtner
Copy link
Contributor

fixes KILTProtocol/ticket#2717

The config package is build mainly as cjs, with only an additional index.mjs simplifying imports from esm. This is to make sure that only one file holds all state, so that any settings that are made are respected by all imports of the package, as long as there is no package duplication.

How to test:

This issue prevented using the sdk as a transitive dependency of a package that was built as cjs in a project that was esm based (sporran). I verified that with this change, this is now possible.

Checklist:

  • I have verified that the code works
  • I have verified that the code is easy to understand
    • If not, I have left a well-balanced amount of inline comments
  • I have left the code in a better state
  • I have documented the changes (where applicable)
    • Either PR or Ticket to update the Docs
    • Link the PR/Ticket here

@rflechtner rflechtner requested review from arty-name and lukeg90 May 23, 2023 12:35
@rflechtner rflechtner self-assigned this May 23, 2023
Copy link
Collaborator

@arty-name arty-name left a comment

Choose a reason for hiding this comment

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

The approach looks a little backwards to me, but not having the capacity to fix Sporran right now, I approve it

@rflechtner
Copy link
Contributor Author

The approach looks a little backwards to me, but not having the capacity to fix Sporran right now, I approve it

It's not just about sporran - any esm consumer of the kilt extension library would have issues with this, and I suspect the issue could come back to bite us in the ass at any time. Better to fix the root cause once and for all I'd say.

@rflechtner rflechtner merged commit c8e574a into develop May 23, 2023
@rflechtner rflechtner deleted the rf-fix-config branch May 23, 2023 16:31
rflechtner added a commit that referenced this pull request May 23, 2023
* fix: make sure config is singleton
rflechtner added a commit that referenced this pull request May 23, 2023
* fix: make sure config is singleton
rflechtner added a commit that referenced this pull request May 23, 2023
* fix: make sure config is singleton
@arty-name
Copy link
Collaborator

While other consumers could have the same misconfiguration as Sporran, they could also be fixed, I assume

@rflechtner
Copy link
Contributor Author

Is using ESM as a module system 'a misconfiguration'? I see the entire ecosystem shifting towards it, seems like an odd characterisation to me

@arty-name
Copy link
Collaborator

Configuring the bundler in a way that it loads the same package twice in different versions is a misconfiguration. This is just webpack behaviour, and another bundler is likely to behave differently. It’s on a totally different level than Node.js stuff. Webpack should have only taken the ESM version.

rflechtner added a commit that referenced this pull request May 24, 2023
* fix: make sure config is singleton
@rflechtner rflechtner mentioned this pull request May 24, 2023
27 tasks
@rflechtner
Copy link
Contributor Author

Configuring the bundler in a way that it loads the same package twice in different versions is a misconfiguration. This is just webpack behaviour, and another bundler is likely to behave differently. It’s on a totally different level than Node.js stuff. Webpack should have only taken the ESM version.

That is true, if you are not using a bundler though you would get the same behaviour. Node.js would follow the esm path from an esm dependency and the cjs path from a cjs dependency, and thus also import two versions of the same package.

@arty-name
Copy link
Collaborator

I don’t think we have a CJS dependency for node yet, and if one is in the works, it could be done ESM-compatible, I guess. This would be future-proof.

@arty-name
Copy link
Collaborator

By the way, check out the reason Sporran build fails for the new SDK

@rflechtner
Copy link
Contributor Author

By the way, check out the reason Sporran build fails for the new SDK

is tree shaking not working for cjs code?

@arty-name
Copy link
Collaborator

I have not investigated the exact root cause, but I think this showcases that the benefits of CJS-only approach are not as clear-cut as expected

@rflechtner
Copy link
Contributor Author

I mean it makes sense, if the problem was before that your bundler couldn't deduplicate the ESM/CJS versions of the config package, it's now probably failing to deduplicate the imports of the config package. At least it doesn't break now, but there's a ton of dead code added. I could remove dependencies of the package that we don't really need, if you think that helps?

@arty-name
Copy link
Collaborator

I need to investigate it first

rflechtner added a commit that referenced this pull request Jul 26, 2023
* fix: make sure config is singleton
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.

2 participants