-
Notifications
You must be signed in to change notification settings - Fork 410
fix(localizations): Fix building of retheme subpath exports #2245
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
Conversation
🦋 Changeset detectedLatest commit: b4e4326 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| entry: uiRetheme | ||
| ? ['src', '!src/index.ts', '!src/en-US.ts'] | ||
| : ['src', '!src/index.retheme.ts', '!src/en-US.retheme.ts'], | ||
| entry: { |
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.
Why wouldn't this work?
entry: uiRetheme ? ['./src/*.ts', '!./src/index.ts', '!./src/en-US.ts'] : ['./src/*.ts', '!./src/index.retheme.ts', '!./src/en-US.retheme.ts']
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.
Before merging the change with the subpath exports the build output contained an index entry point file mapped to bundled index.ts or index.retheme.ts depending on the retheme flag.
With the change to support subpapths we are exporting an index or index.retheme entrypoint. The packages depending on the localizations depend on the index entrypoint even when retheme is enabled.
| 'ar-SA': 'src/ar-SA.ts', | ||
| 'cs-CZ': 'src/cs-CZ.ts', | ||
| 'da-DK': 'src/da-DK.ts', | ||
| 'de-DE': 'src/de-DE.ts', | ||
| 'el-GR': 'src/el-GR.ts', | ||
| 'en-US': 'src/en-US.ts', | ||
| 'es-ES': 'src/es-ES.ts', | ||
| 'fr-FR': 'src/fr-FR.ts', | ||
| 'he-IL': 'src/he-IL.ts', | ||
| 'it-IT': 'src/it-IT.ts', | ||
| 'ja-JP': 'src/ja-JP.ts', | ||
| 'ko-KR': 'src/ko-KR.ts', | ||
| 'nb-NO': 'src/nb-NO.ts', | ||
| 'nl-NL': 'src/nl-NL.ts', | ||
| 'pl-PL': 'src/pl-PL.ts', | ||
| 'pt-BR': 'src/pt-BR.ts', | ||
| 'pt-PT': 'src/pt-PT.ts', | ||
| 'ro-RO': 'src/ro-RO.ts', | ||
| 'ru-RU': 'src/ru-RU.ts', | ||
| 'sk-SK': 'src/sk-SK.ts', | ||
| 'sv-SE': 'src/sv-SE.ts', | ||
| 'tr-TR': 'src/tr-TR.ts', | ||
| 'uk-UA': 'src/uk-UA.ts', | ||
| 'vi-VN': 'src/vi-VN.ts', | ||
| 'zh-CN': 'src/zh-CN.ts', | ||
| 'zh-TW': 'src/zh-TW.ts', |
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.
Reading the description of the PR I still don't understand why that is necessary?
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.
We need a way to define in the tsup config that the dist/index should be generated by the src/index.retheme.ts for retheme flag is enabled and the src/index.ts when the flag is disabled.
I couldn't find a way to make that condition work with the entry without passing a Record matching the entry point and the file ref.
943891e to
d208107
Compare
17fe7f8 to
aff8411
Compare
Before the localizations subpath exports change we had a single entrypoint, the `index`. Based on the CLERK_RETHEME env we were using the index.retheme.ts or the index.ts as entrypoint during build. To support that type of conditional output for all the exported subpaths we need to either define a callback to replace the *.retheme files with the *.ts files or conditional choose them in buiild configuration. Due to some issues with the tsup `onSuccess` callback i manually defined all the subpath exports. We need to be careful in the future and add the new localization files in both package.json#files, tsup.config.ts and in subpaths.mjs file to support subpath exports successfully
aff8411 to
b4e4326
Compare
|
It'd do the following instead:
|
This is not going to work as we're also changing the types and dead-code elimination will not work in this case. |
Description
This PR fixes the failing build of localization package when using Retheme variant.
Before the localizations subpath exports changes we had a single entrypoint defined in our tsup config, the
index. As part of the Retheme Project we have introduced theCLERK_RETHEMEenv to support using theindex.retheme.tsinstead ofindex.tsas the entrypoint.To implementa the conditional usage of
*.rethemefiles for all the exported subpaths we need toeither define a callback to replace the files with the
*.tsfiles with the*.retheme.tsfiles or define all those conditions into a map in ourtsup.config.ts.Due to some issues with the tsup
onSuccesscallback,i manually defined all the subpath exports.We need to be careful in the future and add the new localization files in both package.json#files, tsup.config.ts and in subpaths.mjs file to support subpath exports successfully.
Checklist
npm testruns as expected.npm run buildruns as expected.Type of change
Packages affected
@clerk/backend@clerk/chrome-extension@clerk/clerk-js@clerk/clerk-expo@clerk/fastifygatsby-plugin-clerk@clerk/localizations@clerk/nextjs@clerk/clerk-react@clerk/remix@clerk/clerk-sdk-node@clerk/shared@clerk/themes@clerk/typesbuild/tooling/chore