Skip to content

test: switch to experimental built-in type stripping #65

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

AugustinMauroy
Copy link
Member

@AugustinMauroy AugustinMauroy commented Jun 13, 2025

  • test suite time 29s => 25s

@AugustinMauroy
Copy link
Member Author

test are failing for something look like not related to this pr

@aduh95
Copy link
Contributor

aduh95 commented Jun 13, 2025

test are failing for something look like not related to this pr

What makes you think that? AFAICT it's definitely related to this PR

@aduh95 aduh95 changed the title test: use native ts test: switch to experimental built-in type stripping Jun 13, 2025
@AugustinMauroy
Copy link
Member Author

my bad

@aduh95
Copy link
Contributor

aduh95 commented Jun 15, 2025

I'm getting the following error when I try your branch:

$ yarn build --force  
packages/crypto/tsconfig.json:4:35 - error TS5096: Option 'allowImportingTsExtensions' can only be used when either 'noEmit' or 'emitDeclarationOnly' is set.

4     "allowImportingTsExtensions": true,
                                    ~~~~


Found 1 error.

@aduh95
Copy link
Contributor

aduh95 commented Jun 15, 2025

BTW I wonder if a better approach would be to split it into several PRs:

  1. Switch the custom loader to use the built-in type stripping instead of sucrase.
  2. Remove custom test file?
  3. Update the specifiers in the source files.
  4. Remove the loader eventually.

@AugustinMauroy
Copy link
Member Author

BTW I wonder if a better approach would be to split it into several PRs:

  1. Switch the custom loader to use the built-in type stripping instead of sucrase.
  2. Update the specifiers in the source files.
  3. Remove the loader eventually.
  1. Maybe we should put the build in the CI.
  2. I'm not in favor of multiple PRs because the work isn't huge and 80% is already done. All that's left is to fix the build.

@aduh95
Copy link
Contributor

aduh95 commented Jun 15, 2025

I'm not in favor of multiple PRs because the work isn't huge and 80% is already done. All that's left is to fix the build.

How much of the job is done is kinda irrelevant, it should be split nonetheless IMO

@aduh95
Copy link
Contributor

aduh95 commented Jun 16, 2025

I've opened #68 to cover the first step

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

Successfully merging this pull request may close these issues.

2 participants