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

chore: build a valid esm and cjs output #861

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

neilcampbell
Copy link

@neilcampbell neilcampbell commented Mar 20, 2024

Produce a valid ESM and CJS output, so importing into an ESM project doesn't fallback to using the CJS version.

This fixes the following error, when using algosdk in an ESM project.

SyntaxError: Named export 'Indexer' not found. The requested module 'algosdk' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from 'algosdk';
const { Indexer } = pkg;

I have tested that the following imports, which were previously resolvable still work correctly.

import { tealSign, modelsv2, Algodv2, getApplicationAddress } from 'algosdk'
import type { BaseHTTPClientResponse, Query } from 'algosdk/dist/types/client/baseHTTPClient'
import { BaseHTTPClientResponse, Query } from 'algosdk/src/client/baseHTTPClient'
import { sign, PUBLIC_KEY_LENGTH } from 'algosdk/dist/types/nacl/naclWrappers'
import english from 'algosdk/dist/types/mnemonic/wordlists/english'
import { AppClearStateTxn, MultisigMetadata } from 'algosdk/dist/types'

Additionally I have tested the update in AlgoKit utils-ts and all tests pass.

@robdmoore
Copy link

Worth also testing the imports in subscriber-ts too if possible, there's a couple of weird ones in there.

@neilcampbell
Copy link
Author

@robdmoore Can confirm the AlgoKit subscriber-ts imports work correctly with this change and all tests pass.

@neilcampbell
Copy link
Author

Just pushed an update to the PR to include a couple of additional index imports I missed. This update handles the below imports:

import { ABIContractNetworkInfo } from 'algosdk/dist/types/abi'
import { AssetFreezeTxn } from 'algosdk/dist/types/types/transactions'
import { AssetFreezeTransaction } from 'algosdk/dist/types/types/transactions/asset'

Copy link
Member

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

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

Generally speaking I'm hesitant to accept changes that differ from what's already been added to the 3.0.0 branch in #836

I think I can accept tsc-alias, since it does simplify things on this branch, but at the moment I am against the ./dist and ./src additional export paths.

"import": "./dist/esm/index.js",
"require": "./dist/cjs/index.js"
},
"./dist/types": {
Copy link
Member

Choose a reason for hiding this comment

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

Why/how did you decide to export these additional paths? Clients should not be importing anything besides the base library.

Copy link
Author

Choose a reason for hiding this comment

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

It's purely to maintain compatibility with the current 2.x.x version, so as to not introduce any breaking changes. Because there is no exports defined, users can import anything. dist and src are both directories that are in the package, so technically can be imported by a user.

For example algokit-utils-ts uses the following import as it's not exported from an index. https://github.com/algorandfoundation/algokit-utils-ts/blob/b5ae35781796cbf2196916e83e0712229349ca4f/src/indexer-lookup.ts#L2.

Copy link
Member

Choose a reason for hiding this comment

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

As maintainers, it's not practical for us to support these import paths. I'd much rather export the types that need to be exported through the main package than support these additional import paths.

Alternatively, I'd also be ok supporting the same export paths as in the 3.0.0 branch, though it may take some work to get that working on this branch

Copy link
Author

Choose a reason for hiding this comment

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

I do agree, however the package already allows users to import these paths, so the maintainers are technically already supporting these.

The trouble I have is that if we do change the import paths, it is a breaking change and should be signalled as such via a major version bump. I'm assuming that isn't desirable either, due to the existing 3.0.0 branch?

package.esm.json Outdated Show resolved Hide resolved
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

3 participants