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

Universal Bundle #66

Merged
merged 10 commits into from
Jul 1, 2024
Merged

Universal Bundle #66

merged 10 commits into from
Jul 1, 2024

Conversation

bh2smith
Copy link
Collaborator

@bh2smith bh2smith commented Jun 12, 2024

Some projects have been struggling to import from this package because of some node target stuff I do not understand.

Example project: BitteProtocol/near-safe#1

/nearly-safe/node_modules/near-ca/dist/index.js:1
export * from "./chains/ethereum";
^^^^^^

SyntaxError: Unexpected token 'export'

I have tried to repackage the project and locally tested with npm link. Will publish a beta release from this branch and proceed.

cc @nlordell

Note that somehow also this change wound up formatting from previously unformulated files.

Copy link

@nlordell nlordell left a comment

Choose a reason for hiding this comment

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

I think CommonJS is kind of old school. I expect you just need to make your other project type: module in order to get things to work.

@bh2smith
Copy link
Collaborator Author

bh2smith commented Jun 12, 2024

I expect you just need to make your other project type: module in order to get things to work.

I tried that... it was a no go with the other conflicting configuration issue...
Specifically, it conflicted with whatever error came from this:
https://github.com/bh2smith/nearly-safe/blob/5506755265149f6a26df818e77b1f3d4f02b2393/index.ts#L2

I would love to see a better alternative, but for now at least this removes the conflict.

Or rather, until the other project has some non-conflicting configuration (safe imports with these imports) at least this adjustment removes the conflict.

@nlordell
Copy link

nlordell commented Jun 13, 2024

I looked at how some other projects do it, and some include both ES and CommonJS dist files and have entries in the package.json for this: https://nodejs.org/api/packages.html#conditional-exports

The aforementioned link explicitly provides a way to support both files for use with require and with import:

// package.json
{
  "exports": {
    "import": "./index-module.js",
    "require": "./index-require.cjs"
  },
  "type": "module"
}

This probably requires two TS build steps, but that shouldn't be a big deal (and don't forget you can "inherit" ts config files for each module system you want to support).

Edit: there is a section on dual CommonJS/ES module packages: https://nodejs.org/api/packages.html#dual-commonjses-module-packages

@bh2smith
Copy link
Collaborator Author

That's great news I'll have to look into it. However I don't believe there is any reason why this project or anyone wants to support JS "require" imports if that's what all this is about. These conflicting imports have only ever come up in typescript projects.

Note that I don't know what the changes proposed here actually mean but I am beginning to interpret it as "CommonJS" is somehow related to the import syntax convention. Up until now I was thinking these changes would make the package more universally compatible - but it doesn't need to be compatible with old JavaScript by any means.

@nlordell
Copy link

So the issue is that your TypeScript setup is generating JavaScript code with requires in them (i.e. "old" JavaScript, or CommonJS). This was my point about:

I think CommonJS is kind of old school

You are basically changing this project to use the "old school" way of requiring module dependencies instead of ES modules.

The above link describes how you can support both, so it would be providing more compatibility.

@bh2smith
Copy link
Collaborator Author

bh2smith commented Jul 1, 2024

This problem is still very prevalent causing issues with transformation of near-safe into an npm package:
BitteProtocol/near-safe#23

In fact, there are no issues with this for build it only becomes a problem when trying to run the script file.

@bh2smith
Copy link
Collaborator Author

bh2smith commented Jul 1, 2024

Ok - Finally got something compatible thanks to the help of @jfschwarz and his amazing suggestion to use https://github.com/privatenumber/tsx!

@bh2smith bh2smith merged commit d837525 into main Jul 1, 2024
2 checks passed
@bh2smith bh2smith deleted the universal-bundle branch July 1, 2024 13:04
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