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

feat(config): support typescript configs #1233

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

Conversation

Titozzz
Copy link

@Titozzz Titozzz commented Mar 13, 2024

Summary

Allowing the user to user ts config will reduce configuration mistakes

I had to revert 05b9ed4 as jiti uses os.tmpdir and it would conflict. But it seems to be running fine with node 18/20 at least

Test plan

Unsure how to test this, but the code seems pretty straightforward
Happy to discuss if testing is needed

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Mar 13, 2024
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.45%. Comparing base (9151a87) to head (cb56c63).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1233   +/-   ##
=======================================
  Coverage   83.45%   83.45%           
=======================================
  Files         207      207           
  Lines       10712    10713    +1     
  Branches     2669     2669           
=======================================
+ Hits         8940     8941    +1     
  Misses       1772     1772           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@robhogan robhogan left a comment

Choose a reason for hiding this comment

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

Thanks for this, this would be a nice feature.

I do have some concerns about the dependency on jiti here - it's a very oddly assembled package that claims zero dependencies, but actually bundles (some version of) Babel as a 1.6MB file, discarding its licence in the process: https://unpkg.com/browse/jiti@1.21.0/dist/ .

That, and the fact that it's modifying the environment enough to break tests in weird ways, means we really can't accept a dependency on it.

As a possible alternative, transforming files is somewhat within Metro's wheelhouse - we obviously already have a dependency on Babel - and based on https://github.com/Codex-/cosmiconfig-typescript-loader/blob/main/lib/loader.ts it looks like implementing a loader is pretty straightforward - if it's only a few lines (plus tests), and a Babel plugin to roll our own, that might be the best option.

Aside: We're planning to update cosmiconfig for ESM support but are currently blocked by a Node<20 bug, details: react-native-community/cli#2167 (comment)

Allowing the user to user ts config will reduce configuration mistakes

Curious what you mean by this - I can see that TS would be nicer both syntactically and for consistency, but TS should already be checking the JSDoc types in metro.config.js - it's a template bug if not. Was it not working for you?

2. `metro.config.json`
3. The `metro` field in `package.json`
2. `metro.config.ts`
3. `metro.config.cjs`
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch - we should land this docs fix regardless.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants