-
Notifications
You must be signed in to change notification settings - Fork 90
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
Switch from rollup to vite, from commonjs to ecmascript modules #1740
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1740 +/- ##
=======================================
Coverage 98.20% 98.21%
=======================================
Files 18 17 -1
Lines 1562 1565 +3
Branches 333 333
=======================================
+ Hits 1534 1537 +3
Misses 28 28 ☔ View full report in Codecov by Sentry. |
a83e48d
to
bb95a1c
Compare
bb95a1c
to
7b440ed
Compare
Hey @brunoocasali, it's been a while. Please, can you take a look at this PR? It would unblock me from contributing even more, for instance using |
I support this change too 👍 However, I would recommend making this non-breaking for users. For the exports: I would add information in the documentation to explain that default export is deprecated, will be removed one day, and named export should be preferred. For the UMD bundle: is there a way to approach it in the same way? Via documentation, and roll out the breaking change in the future. |
Thanks for the feedback, I'll try to do just that. |
1821: [Playground] Replace Parcel by Vite r=flevi29 a=Strift # Pull Request ## Related issue Fixes #1820 ## What does this PR do? - Update the playground to use a modern bundler, Vite (which is also in line with upcoming changes in #1740) ## PR checklist Please check if your PR fulfills the following requirements: - [x] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)? - [x] Have you read the contributing guidelines? - [x] Have you made sure that the title is accurate and descriptive of the changes? Thank you so much for contributing to Meilisearch! Co-authored-by: Strift <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @flevi29, thanks so much for this PR 🙌
Most things look good after testing things on my end. I have some questions before moving forward with reviewing the rollup/vite configuration.
Filename imports: Why are you importing from "{filename}.js" files even when the extension is ".ts"? Wouldn't it be clearer to import for "filename.ts" instead?
Type file: Is there any reason why you renamed types/index.ts
to types/types.ts
? I think the former file name is more conventional.
vite.config.js
Outdated
// make sure external imports that should not be bundled are listed here for CJS build | ||
external: ["node:crypto"], | ||
} | ||
: // https://github.com/vitejs/vite/issues/11624 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for figuring that out.
Can you please add two comment?
- One comment explaining what this code does, e.g. enable the window.Meilisearch syntax
- One TODO comment so we know where to look to remove this in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did it!
Because TypeScript doesn't rewrite extensions: https://www.typescriptlang.org/docs/handbook/modules/theory.html#module-resolution-for-bundlers-typescript-runtimes-and-nodejs-loaders
There was an index file for a single |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filename imports: Why are you importing from "{filename}.js" files even when the extension is ".ts"? Wouldn't it be clearer to import for "filename.ts" instead?
Because TypeScript doesn't rewrite extensions: https://www.typescriptlang.org/docs/handbook/modules/theory.html#module-resolution-for-bundlers-typescript-runtimes-and-nodejs-loaders
This seems relevant when compiling through TypeScript only. But in our case, we are bundling our code with Vite.
The docs you shared mention this scenario:
But what if there is no output JS file? What if you’re in one of these situations:
- You’re bundling this code, the bundler is configured to transpile TypeScript files in-memory, and it will eventually consume and erase all the imports you’ve written to produce a bundle.
- You’re running this code directly in a TypeScript runtime like Deno or Bun.
- You’re using
ts-node
,tsx
, or another transpiling loader for Node.In these cases, you can turn on
noEmit
(oremitDeclarationOnly
) andallowImportingTsExtensions
to disable emitting unsafe JavaScript files and silence the error on.ts
-extensioned imports.
The README update is sufficient for now, I will update it in a follow-up PR.
README.md
Outdated
@@ -89,6 +93,7 @@ Usage in an HTML (or alike) file: | |||
```html | |||
<script src='https://cdn.jsdelivr.net/npm/meilisearch@latest/dist/bundles/meilisearch.umd.js'></script> | |||
<script> | |||
// also works with window.MeiliSearch, meilisearch.MeiliSearch, window.meilisearch.MeiliSearch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove the comment and only use the recommended way?
But we aren't bundling the ESM version, that's handled & transformed by TypeScript. The whole point of ESM is that it finally has a module system for the web, so no bundling is required. CJS also has a module system (incompatible with the web) ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we aren't bundling the ESM version, that's handled & transformed by TypeScript. The whole point of ESM is that it finally has a module system for the web, so no bundling is required. CJS also has a module system (incompatible with the web) (require), but it's not statically analyzable like ESM import is. And TypeScript already does a lot of work on analyzing the code, which it uses to also transform the code, so I thought it'd be the best to use it to output the ESM version.
Alright, that makes sense! I eventually reached that conclusion as I looked into the build process, but forgot to update this part of my review.
Thanks for the feedback and prompt follow-up.
Let's get this merged 🔥
bors merge |
Build succeeded:
|
1826: Update version for the next release (v0.48.0) r=curquiza a=meili-bot _This PR is auto-generated._ The automated script updates the version of meilisearch-js to a new version: "v0.48.0" CHANGELOGS 👇 ##⚠️ Breaking changes * Improve `token.ts` and make it tree-shakeable (#1661) `@flevi29` **Migration** Replace: ```ts await generateTenantToken("74c9c733-3368-4738-bbe5-1d18a5fecb37", [], { apiKey: "0a6e572506c52ab0bd6195921575d23092b7f0c284ab4ac86d12346c33057f99", expiresAt: new Date("December 17, 4000 03:24:00"), }); ``` By: ```ts await generateTenantToken({ apiKey: "0a6e572506c52ab0bd6195921575d23092b7f0c284ab4ac86d12346c33057f99", apiKeyUid: "74c9c733-3368-4738-bbe5-1d18a5fecb37", searches: [], expiresAt: new Date("December 17, 4000 03:24:00"), }); ``` ## ⚙️ Maintenance * Switch from rollup to vite, from commonjs to ecmascript modules (#1740) `@\flevi29` * Remove mention of support of Node 14 and 16 (#1805) `@\Strift` * Fix and improve `eslint` config, other minor changes (#1795) `@\flevi29` * Fix swapped args in localized-attributes sample (#1818) `@\ellnix` * [Playground] Replace Parcel by Vite (#1821) `@\Strift` * Switch from rollup to vite, from commonjs to ecmascript modules (#1740) `@\flevi29` * Remove `nodemon` dependency (#1822) `@\flevi29` Thanks again to `@\Strift,` `@\ellnix,` and `@\flevi29` 🎉 Co-authored-by: meili-bot <[email protected]>
1826: Update version for the next release (v0.48.0) r=curquiza a=meili-bot _This PR is auto-generated._ The automated script updates the version of meilisearch-js to a new version: "v0.48.0" CHANGELOGS 👇 ##⚠️ Breaking changes * Improve `token.ts` and make it tree-shakeable (#1661) `@flevi29` **Migration** Replace: ```ts await generateTenantToken("74c9c733-3368-4738-bbe5-1d18a5fecb37", [], { apiKey: "0a6e572506c52ab0bd6195921575d23092b7f0c284ab4ac86d12346c33057f99", expiresAt: new Date("December 17, 4000 03:24:00"), }); ``` By: ```ts await generateTenantToken({ apiKey: "0a6e572506c52ab0bd6195921575d23092b7f0c284ab4ac86d12346c33057f99", apiKeyUid: "74c9c733-3368-4738-bbe5-1d18a5fecb37", searches: [], expiresAt: new Date("December 17, 4000 03:24:00"), }); ``` ## ⚙️ Maintenance * Switch from rollup to vite, from commonjs to ecmascript modules (#1740) `@\flevi29` * Remove mention of support of Node 14 and 16 (#1805) `@\Strift` * Fix and improve `eslint` config, other minor changes (#1795) `@\flevi29` * Fix swapped args in localized-attributes sample (#1818) `@\ellnix` * [Playground] Replace Parcel by Vite (#1821) `@\Strift` * Switch from rollup to vite, from commonjs to ecmascript modules (#1740) `@\flevi29` * Remove `nodemon` dependency (#1822) `@\flevi29` Thanks again to `@\Strift,` `@\ellnix,` and `@\flevi29` 🎉 Co-authored-by: meili-bot <[email protected]>
Pull Request
Related issue
Fixes #1626
Waiting on #1739What does this PR do?
rollup
tovite
, which internally usesrollup
, but makes things a lot shorter and simplervitest.config.js
scripts/file-size.js
, packageskleur
,pretty-bytes
,brotli-size
,gzip-size
, as it prints sizes by defaultrollup
plugins and dependencies (@rollup/plugin-terser
,@babel/core
,@babel/preset-env
,@rollup/plugin-babel
,@rollup/plugin-commonjs
,@rollup/plugin-json
,@rollup/plugin-node-resolve
,rollup-plugin-typescript2
)shx
package, andcleanup
script, as it clearsdist
itself"type": "module"
).js
project file now needs to be written in ESM syntax, the web standard syntaxtsconfig.json
according to https://www.typescriptlang.org/docs/handbook/modules/guides/choosing-compiler-options.html#im-writing-a-librarymodule
tonode16
andverbatimModuleSyntax
totrue
, this requires changes to all TypeScript files so they may be compatible with the web standard ESM.js
extensionindex
directory imports, as that is only a Node.js thing, the web doesn't support ittype
keywordWarning
window
with the exports in the future, instead it will only extend it with propertymeilisearch
which contains the exports Do not extendwindow
with all of the exports in UMD bundle, rather just a property ofwindow
#1806Old
New
PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!