-
Notifications
You must be signed in to change notification settings - Fork 280
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(localizations): Fix building of retheme subpath exports #2245
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_RETHEME
env to support using theindex.retheme.ts
instead ofindex.ts
as the entrypoint.To implementa the conditional usage of
*.retheme
files for all the exported subpaths we need toeither define a callback to replace the files with the
*.ts
files with the*.retheme.ts
files or define all those conditions into a map in ourtsup.config.ts
.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.
Checklist
npm test
runs as expected.npm run build
runs as expected.Type of change
Packages affected
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/clerk-expo
@clerk/fastify
gatsby-plugin-clerk
@clerk/localizations
@clerk/nextjs
@clerk/clerk-react
@clerk/remix
@clerk/clerk-sdk-node
@clerk/shared
@clerk/themes
@clerk/types
build/tooling/chore