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

RFC0721: Golden Template for create-react-native-library #721

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

Conversation

atlj
Copy link

@atlj atlj commented Oct 20, 2023

Summary

This RFC aims to document the currently implicit React Native library API and provides a default implementation for it. The RFC also addresses the current challenges caused by the absence of an official library template for React Native, and outlines technical specifications for the proposed template.

The template aims to utilize the existing community tool create-react-native-library and proposes necessary adjustments to the templates and UX to match the specifications outlined in the RFC.

Click here to view the rendered RFC

@atlj atlj changed the title RFC0721: Introducing E2E testing in React Native core RFC0721: Golden Template for create-react-native-library Oct 20, 2023
@atlj atlj marked this pull request as ready for review October 20, 2023 13:58
Copy link
Member

@kelset kelset left a comment

Choose a reason for hiding this comment

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

LGTM. Overall very solid, thanks for all the work! Let's hear some more feedback then I think we can proceed.

proposals/0721-golden-template-crnl.md Outdated Show resolved Hide resolved
proposals/0721-golden-template-crnl.md Outdated Show resolved Hide resolved
## **Unresolved questions**

- How to encourage existing React Native libraries to use the official template?
- How should we tackle the support for out-of-tree platforms such as [React Native Windows](https://github.com/microsoft/react-native-windows)?
Copy link
Member

Choose a reason for hiding this comment

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

good question, I've relayed the RFC internally so that folks from that team can hop in and give their thoughts.

Copy link
Member

Choose a reason for hiding this comment

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

Were there any feedback on this?

Copy link
Member

Choose a reason for hiding this comment

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

nothing has been said, I can ask flag it up once again but I wouldn't consider it a blocker

@atlj atlj requested a review from kelset October 30, 2023 08:31
@atlj
Copy link
Author

atlj commented Dec 1, 2023

Hey @kelset, looks like there are no updates in the discussion, should we proceed?

@kelset
Copy link
Member

kelset commented Dec 4, 2023

From my POV yes, looks like there's no blockers.

@cortinico what do you / Meta thinks? Let's go ahead with it?

proposals/0721-golden-template-crnl.md Outdated Show resolved Hide resolved
## **Unresolved questions**

- How to encourage existing React Native libraries to use the official template?
- How should we tackle the support for out-of-tree platforms such as [React Native Windows](https://github.com/microsoft/react-native-windows)?
Copy link
Member

Choose a reason for hiding this comment

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

Were there any feedback on this?

@cortinico
Copy link
Member

should we proceed?

@atlj what do you mean exactly with "proceed" here? Merge the PR and start implementing it?

@atlj
Copy link
Author

atlj commented Dec 4, 2023

@cortinico exactly, I think we can start the implementation on create-react-native-library

Co-authored-by: Nicola Corti <corti.nico@gmail.com>
@jonthysell
Copy link

jonthysell commented Dec 6, 2023

Just found out about this RFC. I'm actually currently trying to build our (react-native-windows's) new arch lib template on top of the current java-objc template from create-react-native-library. Here's the RNW PR: microsoft/react-native-windows#12481

I just assumed since the RN docs said to use this script that I had to accept its output, but if we're making a dedicated template, here's my two cents from a react-native-windows perspective:

  1. The requirement of a new lib to use a newer newer yarn causes problems, and meant I had to make workarounds:
    1. The inclusion of the local scripts/pod-install.cjs means yarn install simply does not work on Windows, period. It's a complete blocker. My workaround is to comment out that plugin to be able to do any dev work on a Windows dev machine
    2. With the normal react-native app template, I can easily modify the package.json with new dependencies and have node re-call yarn, but if I do that with the lib template, node gets confused and calls classic yarn, breaking the project. My workaround is just to tell folks they'll need to re-run yarn themselves (lame).
    3. It's just confusing when both the react-native and react-native-windows mono-repos both still use classic yarn.
  2. The current template is still pointing to metro-react-native-babel-preset in it's babel config, but will need to be updated to @react-native/babel-preset to go forward (maybe this is just a bug?)
  3. The example app in the template has quite a lot in the metro config, and so does RNW, which makes it an interesting merge from our perspective
  4. There's a lot of "other stuff" in the template, like the built-in commit checks, which, I get why a company might want that, but it doesn't really align with the current react-native app template which doesn't presume such things
  5. The upfront requirement to fill out all the author parts of the package.json is a little much, our CI ends up with something like this just to test creating a new library for our template: npx --yes create-react-native-library@latest --slug testcli --description testcli --author-name "React-Native-Windows Bot" --author-email 53619745+rnbot@users.noreply.github.com --author-url http://example.com --repo-url http://example.com --languages java-objc --type module-new --react-native-version $(reactNativeDevDependency) testcli

@satya164
Copy link
Member

satya164 commented Dec 6, 2023

The inclusion of the local scripts/pod-install.cjs

This is temporary and will be removed once React Native 0.73 is released.

With the normal react-native app template, I can easily modify the package.json with new dependencies and have node re-call yarn

Not sure what you mean by node calling yarn. Can you describe the workflow in more detail?

It's just confusing when both the react-native and react-native-windows mono-repos both still use classic yarn.

yarn 1 has been discontinued for a while and not receiving any updates or bug fixes. dunno why react-native and react-native-windows monorepos still use it, but

  • the monorepo structure that the library generates isn't supported by yarn 1
  • the app template is soon going to use the latest version of yarn

besides, the consumers of the library are very different from the people who are working on react-native or react-native-windows, so it's unlikely that there'll be confusion due to those repos still using the deprecated version.

The current template is still pointing to metro-react-native-babel-preset

This will be updated.

like the built-in commit checks, which, I get why a company might want that, but it doesn't really align with the current react-native app template which doesn't presume such things

Conventional commits are a core part of how the release process is set up, and pre-commit hooks enforce that. Not enforcing the commit message format messes up the whole release setup tooling such as semantic version bumps or changelog generation.

The goal behind starting this project was to provide a preconfigured setup to make it easier for library authors to publish packages, which also means it has an opinionated setup and preconfigured tools such as tools to simplify the release process, ensuring quality contributions by having preconfigured CI config, etc. so that the library authors won't need to waste time configuring these before they can write some code or publish the library.

So far we haven't received any requests to remove those from consumers. And it's easy enough to delete those configs and dependencies anyway as most of that is in package.json. So at this point, I don't want to change anything here. If many people want to remove those, then we can consider making them optional.

The upfront requirement to fill out all the author parts of the package.json is a little much

We autodetect these questions so users rarely have to fill them in manually. Some are essential - will crash pod install if missing, for example. As for CI, passing them in arguments is a one-time thing so I don't think we should be optimizing for the CI use case.

@cortinico
Copy link
Member

There's a lot of "other stuff" in the template, like the built-in commit checks, which, I get why a company might want that, but it doesn't really align with the current react-native app template which doesn't presume such things

I agree with @jonthysell on this point. Ideally the opinions and choices that the react-native template and create-react-native-library imposes should be aligned. Ideally having those as optional, which can be easily included with a selection in the launcher should be preferred.

The example app in the template is

@jonthysell did you intend to say something about the sample app here?

@jonthysell
Copy link

@jonthysell Jon Thysell (JAUNTY) FTE did you intend to say something about the sample app here?

I edited my comment with what I think I was intending to say yesterday. Just that the metro config has a bunch of stuff in it that isn't necessary in the normal app template, and RNW has to add its own stuff to the config because metro (still?) doesn't support per platform configs.


- [https://github.com/brodybits/react-native-module-init (not maintained)](https://github.com/brodybits/react-native-module-init)
- [https://github.com/brodybits/create-react-native-module (not maintained)](https://github.com/brodybits/create-react-native-module)

Copy link

Choose a reason for hiding this comment

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

Would it be appropriate to mention create-expo-module here?

Copy link
Author

Choose a reason for hiding this comment

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

Although it's a similar tool, it doesn't have the exact same functionality as CRNL. AFAIK you cannot create a native library that's compatible with a Bare React Native app using create-expo-module.

Copy link

Choose a reason for hiding this comment

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

I suppose not by itself.

As far as I know, the only extra thing you should need to do to use it in a bare RN app is npx install-expo-modules@latest

Copy link

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

It has a different API than the API of native modules and views in React Native, so it cannot be considered an alternative CLI to create them. It's an alternative way to write modules which is not what this section is meant for.

@vonovak
Copy link
Member

vonovak commented Apr 25, 2024

Hello,
I have a few experience-driven notes that, as a maintainer of a few libs, I felt like sharing. Maybe some of them are covered in the RFC already, or are more suited for create-react-native-library so apologies if it's out of scope.

  1. RNTA (react native test app) - this has become an essential tool for enabling long-term maintenance, as it makes upgrading easy. No one enjoys upgrading, and in the end it blocks development of new stuff and ensuring compatibility with latest RN. Having RNTA by default would be a great enabler in this.

  2. babel-plugin-module-resolver in create-react-native-library

I't used here, and documented here.

I'd suggest that it's removed and instead we use metro resolver itself (extraNodeModules option in config). This:

  • removes a middleman in resolution, making it simpler and more predictable (I spent some time the other confused as to why bundling didn't work in my example project).

  • enables the package to have multiple entries, as specified by exports in package.json without and have that as a single source of truth (https://reactnative.dev/blog/2023/06/21/package-exports-support)

  • removes the need for source field in package.json which afaict is not a standard thing.

  1. DevEx: Even when using RNTA, I have noticed a few libraries using it in a way which, afaik (I might be wrong) doesn't allow the library source code to be hot-reloaded.

Example here. This is because metro isn't even watching the library sources. I'm not sure how the maintainers work on these libraries but I think the general expectation is that hot reloading should "just work".

E.g. RN datetime picker uses a different approach, where it has a single package json, single node_modules - so you don't need to worry about multiple copies of react being found by metro, or multiple versions of @types/react (which flags valid code) and so on - and metro configured so that hot reloading works for library sources as well. I don't see any downsides except that it might look odd / less intuitive to some people. Just trying to show there are different ways the community approaches this, with different pros and cons.

Just my 2 cents :)

@satya164
Copy link
Member

@vonovak

RNTA (react native test app) - this has become an essential tool for enabling long-term maintenance

We want to add it, and have mentioned it in the RFC. However it'll probably be an option since some libraries may need access to the app's native code in case of custom stuff that's not handled by autolinking. There are also concerns that you are no longer testing with a typical React Native app.

Pull requests welcome to add this feature as long as it's an option. Whether it'll default to RNTA or regular app would be up to Meta.

babel-plugin-module-resolver in create-react-native-library
I'd suggest that it's removed and instead we use metro resolver itself (extraNodeModules option in config)

It's not quite the same. We always want to resolve to the source code, not use the node resolution algorithm. In future we'd be removing the react-native field from package.json (necessary to support things like aliases which is highly requested), and also allow optimizations such as skipping compiling libraries in node_modules.

In that case, it'll resolve to built code instead of source code which means you need to rebuild everytime something changes in JS which isn't the DX we're going for.

In the future, we plan to internalize the babel plugin so that we can make other improvements, such as supporting things like aliases, and potential additional dev time checks such as importing something not present in files.

I spent some time the other confused as to why bundling didn't work in my example project

Is there something misconfigured in the template or need to improve documentation and error messages?

removes the need for source field in package.json which afaict is not a standard thing.

source field is also used by Parcel. Though, there's no "need" for it right now either. We just use it since some other tools used it too and seems better to use something other tools use instead of hardcoding to a file or coming up with something ourselves. We can remove it right now, though I think it's better to leave it.

DevEx: Even when using RNTA, I have noticed a few libraries using it in a way which, afaik (I might be wrong) doesn't allow the library source code to be hot-reloaded.
Example here. This is because metro isn't even watching the library

But it's the library making changes, right? Unless something is wrong with the template itself I'm not sure why we need to worry about some projects using it differently that breaks hot reloading. The metro config from the template watches the root folder. If people want to change how things are set up in the template then it's up to them. All we can do is improve documentation if something is unclear.

In the future we plan to internalize the metro config so it won't be something people need to maintain.

E.g. RN datetime picker uses a different approach, where it has a single package json, single node_modules

It also means installing from git installs additional dependencies people may use for the example apps. In addition, it makes it easier to accidentally mix package deps and example app's deps since it's not going to be a common habit to install non-tooling stuff as dev dependency.

This setup also means that you're not testing the library with a typical react-native app structure, so it maybe easier to make mistakes or assumptions that won't work when installing from npm.

Both approaches have their pros and cons. I personally prefer not to mix example app and library-related code, so that's how we have it.

@jamesdome
Copy link

got it. thanks

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.

None yet

8 participants