flevi29 commented 10 months ago

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
- JSDelivr can also look at that field, which might be the reason why it was done like so, but we have jsdelivr field for that, so it'll never look at browser field https://www.jsdelivr.com/documentation#id-configuring-a-default-file-in-packagejson

More details in upcoming PR.

flevi29 commented 10 months ago

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 commented 9 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 5 months ago

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

meilisearch / meilisearch-js

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

Rollup

1616, #1620 is a blocker for this one.