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

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

thisjt
Copy link

@thisjt thisjt commented May 5, 2024

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: #4378

@thisjt thisjt changed the title Add Require and Import Values in package.json for better mjs Support Add Require and Import Values in package.json for better ESM Support May 5, 2024
@dominikg
Copy link

dominikg commented May 5, 2024

thanks for working on improving esm compatibility ❤️

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.

@olifolkerd
Copy link
Owner

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

@olifolkerd
Copy link
Owner

@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

@thisjt
Copy link
Author

thisjt commented May 5, 2024

@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/tabulator-tables@6.2.1

@dominikg
Copy link

dominikg commented May 5, 2024

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.

@olifolkerd
Copy link
Owner

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

@thisjt
Copy link
Author

thisjt commented May 5, 2024

@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.

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