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

fix(javascript): import type for bundler #4020

Merged
merged 2 commits into from
Oct 23, 2024
Merged

Conversation

shortcuts
Copy link
Member

@shortcuts shortcuts commented Oct 22, 2024

🧭 What and Why

🎟 JIRA Ticket: https://algolia.atlassian.net/browse/DI-3102

Changes included:

close algolia/algoliasearch-client-javascript#1565
close algolia/algoliasearch-client-javascript#1562

seems like tsup doesn't remove type imports if the statement doesn't include type in it, leading to javascript files with type imports... see https://www.npmjs.com/package/algoliasearch?activeTab=code in /algoliasearch/dist/node.js L53-L54 for example

also add https://www.typescriptlang.org/tsconfig/#verbatimModuleSyntax to enforce import types consistency

@shortcuts shortcuts self-assigned this Oct 22, 2024
@shortcuts shortcuts requested a review from a team as a code owner October 22, 2024 21:04
@algolia-bot
Copy link
Collaborator

algolia-bot commented Oct 22, 2024

✔️ Code generated!

Name Link
🪓 Triggered by f8b64a08d97187e4b99f67905f749bce33be0b3b
🍃 Generated commit 17b9de6c7999a8c2bfb6d2eeab5405d1485930ab
🌲 Generated branch generated/fix/javascript-exclude-types
📊 Benchmark results

Benchmarks performed on the method using a mock server, the results might not reflect the real-world performance.

Language Req/s
javascript 1648
go 1646
php 1509
csharp 1253
java 1149
python 1058
ruby 931
swift 747

@shortcuts
Copy link
Member Author

seems like verbatimModuleSyntax can help with that, trying it out

@@ -6,8 +6,7 @@
"packages/*"
],
"scripts": {
"build:all": "lerna run build --include-dependencies",
"build:many": "lerna run build --scope 'algoliasearch' --scope '@algolia/requester-testing' --scope '@algolia/logger-console' --scope ${0:-'{@algolia/*,algoliasearch}'} --include-dependencies",
"build": "lerna run build --scope '@algolia/requester-testing' --scope '@algolia/logger-console' --scope 'algoliasearch' --include-dependencies",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now that algoliasearch has everything as a deps, we don't need to select what to run, just ask for algoliasearch

@shortcuts shortcuts enabled auto-merge (squash) October 22, 2024 21:41
Copy link
Contributor

@morganleroi morganleroi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤞🏼

@shortcuts shortcuts merged commit 39a7d52 into main Oct 23, 2024
30 checks passed
@shortcuts shortcuts deleted the fix/javascript-exclude-types branch October 23, 2024 07:22
algolia-bot added a commit that referenced this pull request Oct 23, 2024
algolia-bot added a commit to algolia/algoliasearch-client-javascript that referenced this pull request Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants