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!: remove native components from public API #2412

Merged
merged 2 commits into from
Oct 21, 2024

Conversation

maciekstosio
Copy link
Contributor

@maciekstosio maciekstosio commented Oct 16, 2024

Description

This PR removes native components from our API. The motivation is that those shouldn't be available directly, only through our interface.

Checklist

  • Ensured that CI passes

@maciekstosio maciekstosio marked this pull request as ready for review October 17, 2024 08:35
@kkafar kkafar changed the title refactor: remove native components from API refactor!: remove native components from API Oct 17, 2024
@kkafar kkafar changed the title refactor!: remove native components from API feat!: remove native components from API Oct 17, 2024
Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

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

One thing that concerns me is whether platform extensions, e.g. .web.tsx / .native.tsx are supported by all major bundlers and not only Metro. Do we have knowledge here?

Besides this I think we're good. Just make sure that we work as regular on fresh installation. Thanks!

Comment on lines -60 to -63
/**
* Modules
*/
export { default as NativeScreensModule } from './fabric/NativeScreensModule';
Copy link
Member

Choose a reason for hiding this comment

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

Uff, isn't it consumed in reanimated?

Copy link
Member

Choose a reason for hiding this comment

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

That's a module, not a component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not used anywhere, so I removed that as well. Imo all functions from it should go through our proxy. But I can revert this change.


const ScreenContentWrapper = View;
export default ScreenContentWrapper;
export default View;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export default View;
const ScreenComponentWrapper = View;
export default ScreenComponentWrapper;

Nitpick: only for readability purposes, so that it is clear what component is meant to be exported from this module.

Copy link
Member

Choose a reason for hiding this comment

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

(I suggest doing the same for all the components)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in: 4bc520d

@kkafar kkafar changed the title feat!: remove native components from API feat!: remove native components from public API Oct 17, 2024
@maciekstosio
Copy link
Contributor Author

maciekstosio commented Oct 17, 2024

One thing that concerns me is whether platform extensions, e.g. .web.tsx / .native.tsx are supported by all major bundlers and not only Metro. Do we have knowledge here?

Besides this I think we're good. Just make sure that we work as regular on fresh installation. Thanks!

Is there any other alternative then Re.Pack? Based on https://github.com/callstack/repack/blob/f119ab3eb94eff9d2cc1aec8fcf9f835c3025abc/website/src/public/configs/webpack.config#L77 I assume it handles platform extensions.

Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

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

Looks good

@maciekstosio maciekstosio merged commit d2f7c21 into main Oct 21, 2024
3 checks passed
@maciekstosio maciekstosio deleted the @maciekstosio/Remove-Native-components-from-API branch October 21, 2024 12:59
alduzy added a commit that referenced this pull request Oct 30, 2024
## Description

This PR intents to fix current screen going blank on Android when
returning to the previous one. The issue has been already fixed by
#2134, but
re-appeared recently. After a quick bisect it turned out that
#2412
caused the regression. Bringing back the removed `NativeScreensModule`
export fixes the issue.

Fixes #1685 

## Changes

- added missing export
- modified `Test2282.tsx` repro

## Screenshots / GIFs

### Before


https://github.com/user-attachments/assets/39bf6c66-39d2-4dfd-abc8-ee1c690417a4

### After


https://github.com/user-attachments/assets/003446b2-c976-4cdb-8ab4-348dd619ba79

## Test code and steps to reproduce

- use `Test2282.tsx` repro

## Checklist

- [x] Included code example that can be used to test this change
- [x] Ensured that CI passes

---------

Co-authored-by: Kacper Kafara <kacper.kafara@swmansion.com>
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