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

Update types for require and require.context #41421

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kraenhansen
Copy link
Contributor

Summary:

It's been 1½ years since the merge of facebook/metro#822 and my hope is that this feature is ready to be documented and made available through types.

Changelog:

[GENERAL] [ADDED] - Added types for require.context, which is provided by Metro at build time.

Test Plan:

I've tested this manually in a project.

@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 Nov 11, 2023
@kraenhansen
Copy link
Contributor Author

kraenhansen commented Nov 11, 2023

cc @EvanBacon What do you think? Is this ready for general consumption?
I would love to rely on these types in examples of another package of mine.

It's been 1½ years since the merge of facebook/metro#822 and my hope is that this feature is ready to be documented and made available through types.
@motiz88
Copy link
Contributor

motiz88 commented Nov 13, 2023

I think we need to ship the feature as stable (and on by default) before we ship types that assume it's on.

@kraenhansen
Copy link
Contributor Author

kraenhansen commented Nov 13, 2023

@motiz88 great point! I honestly don't know why I didn't realize this wasn't enabled by default. How about making the context property optional, forcing code to check for it's presence before relying on it? I'll updated the PR accordingly.

@motiz88
Copy link
Contributor

motiz88 commented Nov 13, 2023

Unfortunately I don't think making it nullable is sufficient. We don't support feature-detecting require.context in this way. The best way forward is to enable it out of the box, then document it (including in types).

@kraenhansen
Copy link
Contributor Author

We don't support feature-detecting require.context in this way.

Why wouldn't this work?

if (require.context) {
  require.context(/* ... */);
}

@kraenhansen
Copy link
Contributor Author

Trying to verify this fix this today, I see that this change conflicts with the require global declared by @types/node which are included into the project, at least one place:

This is my experience in VSCode:

Screenshot 2023-11-28 at 18 48 36

@kraenhansen
Copy link
Contributor Author

Because of NodeRequire being declared in @types/node/globals.d.ts and it being automatically included in the project, this PR currently needs a declare const to pin the type of require in user-space:

declare const require: ReactNativeRequire;

if (require.context) {
  require.context('.');
}

I hope to investigate other workarounds for this.

@kraenhansen
Copy link
Contributor Author

Never mind, now I noticed there's runtime code throwing if we simply get context off the require object: I suspect that's the reason we can't feature-detect it 😞 thanks for mentioning this @motiz88.

ERROR Error: The experimental Metro feature require.context is not enabled in your project.
This can be enabled by setting the transformer.unstable_allowRequireContext property to true in your Metro configuration.

Even if we enable this by default, the types won't be accurate if users disable it - perhaps that's good enough?
The TS issue of require being declared as NodeRequire remains 🤔 I'll draft this for now.

@kraenhansen kraenhansen marked this pull request as draft November 28, 2023 20:04
@motiz88
Copy link
Contributor

motiz88 commented Nov 28, 2023

To be clear, at the point when we enable require.context by default (or soon afterwards), we'll probably remove the option entirely.

@karlhorky
Copy link

karlhorky commented Mar 8, 2024

For anyone looking for a copy+paste metroRequire.d.ts for TypeScript types for Metro context.require() to add to your own project until this PR is merged, @EvanBacon has one here:

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.

None yet

4 participants