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

Tabulator ESM with Sveltekit and Vite #4378

Closed
tadmi opened this issue Jan 28, 2024 · 13 comments
Closed

Tabulator ESM with Sveltekit and Vite #4378

tadmi opened this issue Jan 28, 2024 · 13 comments
Labels
Possible Bug A possible bug that needs investigation

Comments

@tadmi
Copy link

tadmi commented Jan 28, 2024

Tabulator 5.5.4
Sveltekit 2.5.0 (built with Vite 5.0.12)

Possible problem
image

and solution: add in tabulator-tables package.json type: module.

The problem appeared with Vite 5.0.8:
specific change — fix(ssr): check esm file with normal file path (#15307) (1597170), closes #15307

@tadmi tadmi added the Possible Bug A possible bug that needs investigation label Jan 28, 2024
@tadmi tadmi changed the title Tabulator with Sveltekit and Vite Tabulator ESM with Sveltekit and Vite Jan 28, 2024
@olifolkerd
Copy link
Owner

Hey @tadmi ,

The challenge with your proposed solution is that Tabulator is not just a module, it offers both ESM and UMD options, which is why you will see in the package.json that it offers both a main and a module.

This works by allowing your task runner to pick whichever version is correct depending on whether you are running as a module or not.

You may need to update your projects package.json to handle this. If i add your proposed solution, it will break it for all UMD users.

I use tabulator in many ESM projects with the current package.json without any issues.

Cheers

Oli :)

@novecento
Copy link

sorry: what does it mean "You may need to update your projects package.json to handle this." ?

@thisjt
Copy link
Contributor

thisjt commented May 3, 2024

I think we need to reopen this issue. Here's a minimum reproducible repo of the issue using SvelteKit.

(File is in src/routes/+page.svelte)
https://github.com/thisjt/tabulator-import-issue

Here is the result on dev run
image

I didn't have this issue when I was on Vite 4, but when I was forced to update to Vite 5 as it is a requirement for SvelteKit 2.0 due to Vercel recommending me to migrate from SvelteKit 1 to 2, this issue started showing up on my imports.

I don't have enough technical expertise for this but tadmi was pointing us to this bug from Vite 4 which relates to astro importing not showing the correct error.
withastro/astro#9388
vitejs/vite#15307

@thisjt
Copy link
Contributor

thisjt commented May 3, 2024

For those wondering what my workaround for this one is, this is how I was importing the TabulatorFull variable from 'tabulator-tables'

import * as Tabulator_Import from 'tabulator-tables';
const { TabulatorFull: Tabulator } = Tabulator_Import;

const transactionTable = new Tabulator('#transactionTable', tableConfig);

@olifolkerd
Copy link
Owner

@thisjt This only happens with the latest version of sveltKit and literally nothing else. I use tabulator in many projects with many different build tools and not a single one throws an error.

Given the 1000s of users of Tabulator if this was a package configuration error of significant size this issues list would be inundated.

If I change the package.json as suggested it will literally break the UMD importing of the package.

I'm not a vite user myself, so don't have the setup to test this one. But if a member of the vite using community wants to come up with a fix and submit a PR then I would be very happy to do a patch release as long as it doesn't break the package for everyone else.

Cheers

Oli :)

@thisjt
Copy link
Contributor

thisjt commented May 4, 2024

Instead of the suggested fix, maybe a wrapper specifically for SvelteKit 2 would be a better (temporary) suggestion. However, it would be kind of a waste of time making it. Let me try raising a ticket on the Vite repo and see what they can find with this issue.

Is it also potentially possible that this issue started showing up because of Svelte 4 removing support for cjs bundling as per PR sveltejs/svelte#8613? This could be why the issue is only showing up specifically on SvelteKit and not on any other Front-end Frameworks.

@thisjt
Copy link
Contributor

thisjt commented May 4, 2024

@olifolkerd we have received some answers with regards to the issue with SvelteKit compatibility. You may check on the linked bug above for some info.

image

I'm currently in the process of creating a repo for reproducing the issue not existing in SvelteKit@1, a barebones project in Vite@5, and fixing the SvelteKit@2 sample repo not working in Stackblitz. He has a different advise with changing the filename of /dist/js/tabulator_esm.js to /dist/js/tabulator_esm.mjs, but not sure how that will affect the entire project.

If this is too much of a risk for a change, maybe the aforementioned SvelteKit wrapper of tabulator would be viable, or honestly I would just document it on the website on my suggested workaround with importing the table specifically for SvelteKit users. At least they won't be left out.

@gatewaynode
Copy link

Just ran into this as well, I tested adding "type": "module", after the "module" declaration on line 8 of the tabulator package.json and this does seem to fix the import issue in SvelteKit 2. Seems this additional K/V here might be fairly safe, or at least worth testing more, want a PR for it?

@olifolkerd
Copy link
Owner

@gatewaynode as described several times above if I did that it would break cjs/umd importing.

There are two different ways to import a package. And the current configuration of the package.json is the approved way to structure a package.json that allows for both cjs and esm importing.

I will not be making any changes that break the importability of this library to resolve an overly strict change in one build environment that is used by a small subset of users.

I'm happy to give the mjs approach outlined above a shot of that resolves things. But again will only include this if it does not break others access

Cheers

Oli

@olifolkerd
Copy link
Owner

@thisjt have you tried the .mjs extension change in your setup? And does it work? (You will need to change the file and possibly update the path in the package.json)

If that does work, then that would also work my end without breaking things. So could be a good opinion.

Cheers

Oli

@thisjt
Copy link
Contributor

thisjt commented May 5, 2024

@olifolkerd I'm glad to say that it worked on my end. Here's what I did. Maybe you need to do some modification on your bundler as you need to build the ESM module as a .mjs extension.

  "module": "dist/js/tabulator_esm.mjs",
  "exports": {
    ".": {
      "require": "./dist/js/tabulator.js",
      "import": "./dist/js/tabulator_esm.mjs"
    }
  },

Changing module only and adding the .mjs extension did not work. We need the exports field in the package.json so that we can properly direct require and import. https://antfu.me/posts/publish-esm-and-cjs

I tried tabulator_esm.js on the import and module field but it throws an Unexpected token 'export' issue, so the rename to .mjs is really needed.

image

I'd be happy to submit a PR for the changes in the Bundler.mjs and package.json if you're fine with that.

@olifolkerd
Copy link
Owner

@thisjt thanks for the testing there.

Yes please submit a PR, I will setup a new branch where I can make the build tweaks which should be small, if you can submit your package tweaks, I can then run some regression tests on other build tools to make sure this doesn't break anything. But it looks good in principle.

Cheers

Oli

@thisjt
Copy link
Contributor

thisjt commented May 5, 2024

Oh wait, lol I forgot to ask you which branch should I be pushing. 🤣 Let me know and I'll reopen a PR.

@olifolkerd I have made the PR for the changes to package.json and build/Bundler.mjs. I'll let you do the building so that you can properly manage the versioning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Possible Bug A possible bug that needs investigation
Projects
None yet
Development

No branches or pull requests

5 participants