-
-
Notifications
You must be signed in to change notification settings - Fork 518
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
refact: remove mixed CJS/ESM, refactorize index.native.tsx #1982
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 🚀 I left some comments.
...props | ||
} = rest; | ||
|
||
if (active !== undefined && activityState === undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we will remove the active
prop and code related to it in v4, just a note for future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks for mentioning that. We can do that in a separate PR 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took a quick look now:
- I like that each component lives in its dedicated source file now <3
- Is there any advantage of converting every function to arrow function?
- We need to make sure that there are no more similar issues to the one we debugged last week (component exported under different name after the refactor)
|
…mansion#1982) ## Description Users are reporting that on Vite they cannot build their project with react-native-screens because of the mixed CJS/ESM files that are being created while building a bundle (you can see [here](https://publint.dev/react-native-screens@3.27.0) that some package managers reports errors about mixed CJS/ESM files). To reduce that behavior I've decided to remove CJS completely while bundling the project, resulting in building only ESM files. Unfortunately because of that I had to remove optional requiring process inside the index.native.tsx file, but this shouldn't have a large impact while using rn-screens. I also decided to move some parts of the screens implementation to separate files - this should improve readability, better understanding of code for newcomers and should improve developer experience overall. Having only imports and exports in index files is also a good practice - this was my main reason why I've planned to do that. Closes software-mansion#1908 - I'll try to ensure that this will fix Vite for sure 👍 ## Changes - Disable bundling CJS files from react-native-screens - Refactorize index.native.tsx files to separate files ## Test code and steps to reproduce First, try to bundle the project - you can see that inside `lib` there shouldn't be `common` directory with the CJS files. Then, try to run FabricTestExample with a couple of tests. Application should work properly as usual. ## Developer notes There are some points that I stumbled upon and I need to mention here. - I've managed to move all of the native components from class to function components, **except**: - **Screen:** Unfortunately we need to stay with class components there, as for now we would like to keep behavior of using `setNativeProps` for a screen (does anybody do that? Or is react-native calling this method for a screen wherever? There's a field for a discussion). - **SearchBar:** Because of managing ref's state and dropping it down to the methods responsible for commands I was also unable to convert this to functional component. - I tried to also refactor index.tsx file, but I see no reason to do this. For now I'm keeping it as it is (with only a slight changes to this file): - Because of a conflict of naming between SearchBarCommands (from types.tsx) and SearchBarCommands as a native component -> it's not that easy to fix, so I suggest fixing this in a future (might be also a good first issue). - I also tried to move `index.native.tsx` to `index.tsx` and to move `index.tsx` to `index.web.tsx`, but because of a conflict I described above and because I don't see the point of rendering conditionally native components depending if `Platform.OS !== 'web'` (and rendering a `View` if Platform.OS is web) I'm keeping the current naming. - Let me know what do you think about the refactor of index.native.tsx! This change is a **Proof of concept** and I codenamed it as a second stage of this PR, so we might give it a try, but I'm all ears about your opinion - IMO it is worth merging . ## Checklist - [X] Ensured that nothing changed while refactoring index.native.tsx - [X] Ensured that CI passes
Description
Users are reporting that on Vite they cannot build their project with react-native-screens because of the mixed CJS/ESM files that are being created while building a bundle (you can see here that some package managers reports errors about mixed CJS/ESM files). To reduce that behavior I've decided to remove CJS completely while bundling the project, resulting in building only ESM files. Unfortunately because of that I had to remove optional requiring process inside the index.native.tsx file, but this shouldn't have a large impact while using rn-screens.
I also decided to move some parts of the screens implementation to separate files - this should improve readability, better understanding of code for newcomers and should improve developer experience overall. Having only imports and exports in index files is also a good practice - this was my main reason why I've planned to do that.
Closes #1908 - I'll try to ensure that this will fix Vite for sure 👍
Changes
Test code and steps to reproduce
First, try to bundle the project - you can see that inside
lib
there shouldn't becommon
directory with the CJS files.Then, try to run FabricTestExample with a couple of tests. Application should work properly as usual.
Developer notes
There are some points that I stumbled upon and I need to mention here.
I've managed to move all of the native components from class to function components, except:
setNativeProps
for a screen (does anybody do that? Or is react-native calling this method for a screen wherever? There's a field for a discussion).I tried to also refactor index.tsx file, but I see no reason to do this. For now I'm keeping it as it is (with only a slight changes to this file):
index.native.tsx
toindex.tsx
and to moveindex.tsx
toindex.web.tsx
, but because of a conflict I described above and because I don't see the point of rendering conditionally native components depending ifPlatform.OS !== 'web'
(and rendering aView
if Platform.OS is web) I'm keeping the current naming.Let me know what do you think about the refactor of index.native.tsx! This change is a Proof of concept and I codenamed it as a second stage of this PR, so we might give it a try, but I'm all ears about your opinion - IMO it is worth merging .
Checklist