tabulator icon indicating copy to clipboard operation
tabulator copied to clipboard

Add Require and Import Values in `package.json` for better `ESM` Support

Open thisjt opened this issue 1 month ago • 7 comments

build/Bunder.mjs has been changed so that tabulator_esm output in dist/js will have the .mjs extension.

package.json has been changed so that the file that the bundler creates is properly referenced.

Referencing bug report: https://github.com/olifolkerd/tabulator/issues/4378

thisjt avatar May 05 '24 08:05 thisjt

thanks for working on improving esm compatibility :heart:

Please keep in mind that adding an exports map is almost always a breaking change and you have to list all exports that are needed.

dominikg avatar May 05 '24 09:05 dominikg

@dominikg do you want to clarify this further? Possibly with any examples of areas of specific concern there?

olifolkerd avatar May 05 '24 11:05 olifolkerd

@thisjt could you revert the change to the bundler please and only include the e package change. Plenty of people still use the .js file directly when pulling from cdns so the .js file cannot be removed without disabling a huge number of users. Instead I will make the bundler duplicate the output to the new extension and deprecate the .js and remove it at the next major version change in a year or two to give people a chance to adapt

olifolkerd avatar May 05 '24 11:05 olifolkerd

@olifolkerd done. You may review the changes and then take over from there.


publint references this error as a breaking change when pkg.module is removed. I think this is not necessary as this is a frontend package and publint says that it's fine to ignore it if not used in a NodeJS environment. Maybe dominik can expound further

pkg.module is used to output ESM, but pkg.exports is not defined. As NodeJS doesn't read pkg.module, the ESM output may be skipped. Consider adding pkg.exports to export the ESM output. pkg.module can usually be removed alongside too. (This will be a breaking change)

If the package isn't meant for Node.js usage, it is safe to ignore this suggestion, but it is still recommended to use "exports" whenever possible. https://publint.dev/[email protected]

thisjt avatar May 05 '24 12:05 thisjt

I don't know the details behind the current structure of this package, just provided links to information how to make this esm compatible. Most ways to do this are a breaking change due to the way it changes how exports of your package can be reached. Without an exports map, everything that is exported can be imported by users. With an exports map, only the entries in the exports map can. publint and the guide by antfu go into more detail about this.

dominikg avatar May 05 '24 13:05 dominikg

@thisjt is the exports bit actually needed? If you point the modules option to the .mjs file does it work in sveltkit?

olifolkerd avatar May 05 '24 14:05 olifolkerd

@olifolkerd yes I assume it's required.

This repo (https://github.com/thisjt/tabulator-import-issue/tree/rename-import) imports (https://github.com/thisjt/tabulator-sveltekit2fix/tree/tabulator-import-rename). This throws an error. This is what its package.json looks like.

...
  "main": "dist/js/tabulator.js",
  "module": "dist/js/tabulator_esm.mjs",
  "sideEffects": [
    "**/*.css",
    "**/*.scss"
  ],
...

This repo (https://github.com/thisjt/tabulator-import-issue/tree/rename-import-with-exports) imports (https://github.com/thisjt/tabulator-sveltekit2fix/tree/tabulator-import-with-exports). This works fine and the table is being rendered correctly. This is what its package.json looks like.

...
  "main": "dist/js/tabulator.js",
  "module": "dist/js/tabulator_esm.mjs",
  "exports": {
    ".": {
      "require": "./dist/js/tabulator.js",
      "import": "./dist/js/tabulator_esm.mjs"
    }
  },
  "sideEffects": [
    "**/*.css",
    "**/*.scss"
  ],
...

I'm not able to provide a stackblitz instance as they don't currently support import from a github repo.

thisjt avatar May 05 '24 15:05 thisjt