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(localizations): Fix building of retheme subpath exports #2245

Merged
merged 1 commit into from
Dec 4, 2023

Conversation

dimkl
Copy link
Contributor

@dimkl dimkl commented Dec 1, 2023

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 the CLERK_RETHEME env to support using the index.retheme.ts instead of index.ts as the entrypoint.
To implementa the conditional usage of *.retheme files for all the exported subpaths we need to
either 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 our tsup.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.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

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

@dimkl dimkl requested a review from anagstef December 1, 2023 12:28
@dimkl dimkl self-assigned this Dec 1, 2023
Copy link

changeset-bot bot commented Dec 1, 2023

🦋 Changeset detected

Latest commit: b4e4326

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 0 packages

When 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: {
Copy link
Member

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']

Copy link
Contributor Author

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.

packages/localizations/tsup.config.ts Outdated Show resolved Hide resolved
Comment on lines 10 to 34
'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',
Copy link
Member

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?

Copy link
Contributor Author

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.

@dimkl dimkl force-pushed the fix-retheme-localizations-build branch 2 times, most recently from 943891e to d208107 Compare December 1, 2023 13:27
@dimkl dimkl requested a review from LekoArts December 1, 2023 15:32
@dimkl dimkl force-pushed the fix-retheme-localizations-build branch from 17fe7f8 to aff8411 Compare December 1, 2023 16:41
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
@dimkl dimkl force-pushed the fix-retheme-localizations-build branch from aff8411 to b4e4326 Compare December 2, 2023 23:36
@LekoArts
Copy link
Member

LekoArts commented Dec 4, 2023

It'd do the following instead:

  1. Define the env var as a boolean in the define block of tsup:
    define: {
      isRetheme: `${uiRetheme}`,
    },
  2. Move the changes from en-US.retheme.ts and index.retheme.ts to their original files and delete those .retheme files
  3. Use if (isRetheme) {} else {} to apply the contents to a let variable
  4. Export that variable and let dead-code elimination remove either the retheme parts or the current parts
  5. Simplify the entry export by just using './src/*.ts'
  6. Win

@dimkl dimkl added this pull request to the merge queue Dec 4, 2023
Merged via the queue into main with commit 9868084 Dec 4, 2023
7 checks passed
@dimkl dimkl deleted the fix-retheme-localizations-build branch December 4, 2023 08:08
@nikosdouvlis
Copy link
Member

It'd do the following instead:

  1. Define the env var as a boolean in the define block of tsup:
    define: {
      isRetheme: `${uiRetheme}`,
    },
  2. Move the changes from en-US.retheme.ts and index.retheme.ts to their original files and delete those .retheme files
  3. Use if (isRetheme) {} else {} to apply the contents to a let variable
  4. Export that variable and let dead-code elimination remove either the retheme parts or the current parts
  5. Simplify the entry export by just using './src/*.ts'
  6. Win

This is not going to work as we're also changing the types and dead-code elimination will not work in this case.
Let's refrain from introducing more build changes for v5 at this point please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants