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

package.json: use file extensions for esmodules compatibility #19

Merged
merged 2 commits into from Jan 21, 2022

Conversation

ryanio
Copy link
Contributor

@ryanio ryanio commented Jan 20, 2022

thanks for this wonderful repository! we use it in @ethereumjs/common

we are just upgrading to esmodules support and found these file extensions necessary to get to the types, we'll put a hotfix in place but thought it would be nice to contribute upstream

for esmodules compatibility
@SheetJSDev
Copy link
Contributor

Somehow we have 3 different packages with 3 different styles 😆

The xlsx package does not include the leading ./:

	"main": "xlsx.js",
	"types": "types/index.d.ts",

The adler-32 package does not have a main extension:

	"main": "./adler32",
	"types": "types/index.d.ts",

This PR is proposing

	"main": "./crc32.js",
	"types": "./types/index.d.ts",

Ideally there'd be one style for the main script and for the types.

Ping @josh-hemphill @ntnyq

@SheetJSDev
Copy link
Contributor

Also, since this is top of mind, would it be helpful to also ship an ESM version? For another project, printj, we ended up making the build script generate an ESM version as well as a normal CJS script

@josh-hemphill
Copy link

josh-hemphill commented Jan 20, 2022

Using explicit extensions is necessary for compatibility with ESM and typescript. I personally like avoiding the use of ./ when practical because it's not a pattern that people coming from windows filesystems are familiar with. But most libraries I've been contributing to have been adding exports fields to their package.json to support all the various usages, which ultimately requires ./ to describe how nested imports should work.

So for libraries, I think just always using ./ is probably the right call. Having the exports field filled out to support various versions of Node.js and fallbacks for cjs and esm is usually a good idea, even if you don't need it yet to add listings for common nested import or internal files people are using.

@ryanio
Copy link
Contributor Author

ryanio commented Jan 21, 2022

that's funny 😄

I also tended not to use relative imports, but I was reading the package.json "exports" proposal (https://github.com/jkrems/proposal-pkg-exports/) and it's recommended there, so I've been using ./

The value of an export, e.g. "./src/moment.mjs", must begin with . to signify a relative path (e.g. "./src" is okay, but "/src" or "src" are not). This is to reserve potential future use for "exports" to export things referenced via specifiers that aren’t relatively-resolved files, such as other packages or other protocols.

It's specifically talking about the values of the "exports" keys, so maybe it doesn't apply here, but I figure why not.

@SheetJSDev
Copy link
Contributor

The official npm docs has an example with main referencing a subdirectory. Search for the code block with "ethopia-waza":

  "main": "lib/waza.js"

@types/lodash has a types field that does not include the leading ./:

  "main": "",
  "types": "index.d.ts",

The "standard" approach seems to be "use file extensions" and "don't prepend with ./"

@ryanio please try it without the ./. If it works in your local flow, please amend the PR and we'll accept:

	"main": "crc32.js",
	"types": "types/index.d.ts",

The adler-32 and printj projects should be changed to follow the same style.

@ryanio
Copy link
Contributor Author

ryanio commented Jan 21, 2022

Sounds good, will try and update, should be fine

edit: yep it worked for me locally!

package.json Outdated Show resolved Hide resolved
@SheetJSDev SheetJSDev merged commit 49af363 into SheetJS:master Jan 21, 2022
@ryanio ryanio deleted the patch-1 branch January 21, 2022 16:57
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.

None yet

3 participants