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

2022-01-20T22:54:54Z

ryanio commented

2022-01-20 22:54:54 +00:00

(Migrated from github.com)

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

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

SheetJSDev commented

2022-01-20 23:09:57 +00:00

(Migrated from github.com)

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

Somehow we have 3 different packages with 3 different styles 😆 The [`xlsx`](https://www.npmjs.com/package/xlsx) package [does not include the leading `./`](https://github.com/SheetJS/sheetjs/blob/master/package.json#L23): ```js "main": "xlsx.js", "types": "types/index.d.ts", ``` The [`adler-32`](https://www.npmjs.com/package/adler-32) package [does not have a `main` extension](https://github.com/SheetJS/js-adler32/blob/master/package.json#L7): ```js "main": "./adler32", "types": "types/index.d.ts", ``` This PR is proposing ```js "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 commented

2022-01-20 23:34:33 +00:00

(Migrated from github.com)

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

Also, since this is top of mind, would it be helpful to also ship an ESM version? [For another project, `printj`](https://github.com/SheetJS/printj/issues/3), we ended up making the build script generate an ESM version as well as a normal CJS script

👍 1

josh-hemphill commented

2022-01-20 23:37:56 +00:00

(Migrated from github.com)

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.

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 commented

2022-01-21 00:08:07 +00:00

(Migrated from github.com)

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.

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 commented

2022-01-21 03:49:00 +00:00

(Migrated from github.com)

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.

[The official npm docs](https://docs.npmjs.com/cli/v8/configuring-npm/package-json#devdependencies) has an example with `main` referencing a subdirectory. Search for the code block with "ethopia-waza": ```js "main": "lib/waza.js" ``` [`@types/lodash` has a `types` field](https://unpkg.com/browse/@types/lodash@4.14.178/package.json) that does not include the leading `./`: ```js "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: ```js "main": "crc32.js", "types": "types/index.d.ts", ``` The [`adler-32`](https://github.com/SheetJS/js-adler32/blob/master/package.json) and [`printj`](https://github.com/SheetJS/printj/blob/master/package.json) projects should be changed to follow the same style.

ryanio commented

2022-01-21 03:51:00 +00:00

(Migrated from github.com)

Sounds good, will try and update, should be fine

edit: yep it worked for me locally!

Sounds good, will try and update, should be fine edit: yep it worked for me locally!

ryanio (Migrated from github.com) reviewed 2022-01-21 04:21:10 +00:00

package.json

ryanio (Migrated from github.com) commented

2022-01-21 04:21:09 +00:00

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

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

Sign in to join this conversation.

No reviewers

No Label

No Milestone

No project

No Assignees

1 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: sheetjs/js-crc32#19