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

Update bundling, transpiling process, move to ESM, change Jest to Vitest #1626

Open
flevi29 opened this issue Jan 13, 2024 · 5 comments
Open
Labels
maintenance Issue about maintenance (CI, tests, refacto...)

Comments

@flevi29
Copy link
Collaborator

flevi29 commented Jan 13, 2024

Rollup

Has many questionable configurations, plugins. TypeScript is related, but for the most part that will be done by #1616.

  • ESM version is apparently meant for bundlers, CJS version for Node.js through npm, these are unnecessarily minified
  • CJS is built twice, once for production and once for development, but with 0 differences
  • The above issue is more to do with how we build, we always build twice no matter what, once for prod and once for dev, should build dev once for local tests and build prod for publishing
  • CJS version doesn't get source maps, but it's what's run in Node.js (because package.json main field)
  • ESM version is meant for bundlers, but current TypeScript configuration and plugin down levels it to ES6 ("target": "ES6"), the thing about down leveled code is that it can no longer be up leveled, the down leveling should mostly be the concern of the package users bundler
  • There's no point in having both minified and un-minifed versions published (there might've been a long time ago, but nowadays we have source maps and JSDelivr even minifies itself the requested file if it's not already minified and doesn't have a .min version laying around)
  • package.json browser field points to the UMD bundle, but this field is primarily meant for bundlers

More details in upcoming PR.

@flevi29
Copy link
Collaborator Author

flevi29 commented Jan 16, 2024

Okay, I might go a little overboard with this one. For all of this to be as good as it can possibly be, it was best for me to move the project to "type": "module", but Jest doesn't like that very much. However I found out that Jest is poorly maintained (at least a lot poorer than in its glory days) these days, and essentially abandoned by Facebook/Meta, so it's probably best to switch to a different testing framework.

I chose Vitest, quite popular and Jest compatible.

Also tests/env for some reason is failing again so I need to fiddle with that too.

Since cross-fetch is CJS only (unless bundled correctly, but even at that it's not done right at the current moment), and we're moving to Node.js 18, 20 which have builtin fetch, I'm dropping cross-fetch. I can see that a custom HTTP request method can be provided, so this shouldn't be an issue for people with outdated runtimes.

I need to know if we really want to support Internet Explorer 11, so I know what version of ES to bundle the UMD to.
(I'm repeating myself here, but currently we are very much NOT supporting Internet Explorer 11, because cross-fetch is bundled incorrectly #1610)

#1616, #1620 is a blocker for this one.

@flevi29 flevi29 changed the title Update bundling, transpiling process Update bundling, transpiling process, move to ESM, change Jest to Vitest Jan 16, 2024
@curquiza curquiza added the maintenance Issue about maintenance (CI, tests, refacto...) label Jan 17, 2024
@flevi29
Copy link
Collaborator Author

flevi29 commented Jan 22, 2024

Also tests/env for some reason is failing again so I need to fiddle with that too.

This was very confusing for me. The files contained a path to the dist UMD bundle, didn't realize at first that they're supposed to be hard links. Now that I know what they are, I could fix them. But it is very much not recommended to have these things in your codebase. http://tdongsi.github.io/blog/2016/02/20/symlinks-in-git/

@brunoocasali
Copy link
Member

I would like to add that any change that might come from this issue or others that change the configurations for the better is very welcomed, but they should work with our existing playgrounds.

Example:

Due to the change made here we introduced an extra step to the end user when they are using Vue3 on Vite (possibly all vite envs).

https://github.com/meilisearch/meilisearch-js-plugins/pull/1294/files#diff-261ca880c173b66e602dd58cd46fa4548456885f268361f0613951e804913069L6

@flevi29
Copy link
Collaborator Author

flevi29 commented May 17, 2024

Yeah, sorry about that, I should've tested the playgrounds as well. I'm not sure why it worked beforehand, I need to investigate further. But I have plans that addresses this as well, the way the code is split up for Node.js and browsers is not ideal, and can cause surprises like the one you mentioned.

@brunoocasali
Copy link
Member

Yeah, that would be awesome if we could improve that and avoid those issues in the future, I'm looking forward to see it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Issue about maintenance (CI, tests, refacto...)
Projects
None yet
Development

No branches or pull requests

3 participants