This repository was archived by the owner on Jun 17, 2021. It is now read-only.
Fix the types of the BN and RLP re-exports#270
Merged
Conversation
Closed
Contributor
|
thanks for the explanation :)
could we just add |
Member
Author
|
I tried your suggestion. It didn't work. I'm not sure why tbh |
ryanio
approved these changes
Aug 1, 2020
Contributor
ryanio
left a comment
There was a problem hiding this comment.
oh that's weird, ok this LGTM for now then!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes #267.
Here's an explanation of what I think was wrong, how I fixed, and how it can be simplified in the future.
BN and RLP are distributed as CJS modules, and use the
module.exports =pattern. This is ok in CJS, but doesn't make sense in ESM.module.exportsis not the same asexport default.TS has a special syntax to do that,
export = obj.BN and RLP typings use that syntax.
To import modules using that syntax, you have to use
import A = require('A')in ts.This module was using
import * as A from 'A'instead, which means something like "import the module namespace" in ESM. Not that module namespace is not a class in ESM, so ts gets confused if you try to initialize it.This worked anyway, as we are just reexporting it, so no error was triggered.
When a TS project tries to use it, ts complains about this error that went unnoticed here.
This make this CSJ/ESM compatibility mess easier, TS introduced the
esModuleInteropflag, which let you useimport A as 'A'to import CSJ modules. This breaks the ESM semantics a bit, but it's a great trade off.As I wrote in a TODO, we are gonna enable that flag in a future version of the ts config package, so this change can be simplified in the future.