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

fix(ios): allow detailed scriptPhases object in config #1611

Merged

Conversation

mikehardy
Copy link
Contributor

align the joi schema for scriptPhases with the actually allowed values

Summary:

react-native-firebase (and react-native-google-mobile-ads) use script phrases that are more than just strings, because we need to depend on the Info.plist file being generated from compilation to run the script

This worked in react-native <= 0.68 but is failing in 0.69.0-rc series

It appears that the validation has changed in the area from t.object to t.string, when it is t.object - our modules react-native-config.js files successfully validate and the modules are auto-linked, when it is t.string they are ignored.

However, if I alter it to t.object in schema.ts, then strings fail, so that's not right.

I attempted to do a joi alternatives with a full definition but I simply could not get the tests to work

So I need help to complete this, please, anyone 🙏 as I'm out of time to work on it, but I'm certain I'm in the right area here and I think I've got the start of a real / fully specified solution

Test Plan:

yarn watch and yarn test --watch was how I was developing

Here is an example of a module that used to work, and should work (and does work with t.object()):

https://github.com/invertase/react-native-firebase/blob/e861bea96b5b54cf56044cb390db539d07ad8f30/packages/app/react-native.config.js#L8-L14

align the joi schema for scriptPhases with the actually allowed values
@mikehardy
Copy link
Contributor Author

@grabbou this is in the area of your refactor, and was mentioned on "road to 0.69" discussion, as I'm in release testers and this was the blocker for me to qualify 0.69 on the Invertase modules, I'm really sorry I couldn't just drop a perfect PR on you, bums me out that I couldn't get this to "just work", it seemed so easy ?

@mikehardy
Copy link
Contributor Author

to be perfectly clear: I won't be upset at anything that happens to this code, by anyone if it can be made to work. Take the commits, take just the code, submit something different yourself, force-push here, no offense taken at all! :-)

@thymikee
Copy link
Member

I'll have a look at this today, thanks for reporting and sketching out a solution. AFAIK scriptPhases were specifically added by @Salakar to support your use case, so it's likely we messed something during refactor.

@mikehardy
Copy link
Contributor Author

All i can say is that it does appear to work if you hack it via patch package to object again, but then tests fail because the tests were just empty array before but now have actual strings in them. If no one else noticed it may be the string style was used by zero consumers as I'm not sure it would have ever worked. Thanks for taking a look though I really appreciate it

@Salakar
Copy link
Member

Salakar commented May 19, 2022

There are a few others in the wild using this also, ones that I know of are: https://github.com/Instabug/Instabug-React-Native/blob/master/react-native.config.js#L5-L10, https://github.com/JaneaSystems/nodejs-mobile-react-native/blob/unstable/react-native.config.js#L9-L26

@mikehardy
Copy link
Contributor Author

Of note! All using the object, not the string style, that's 4 data points (react-native-firebase, react-native-google-mobile-ads, instabug, janea systems) all using object and not string - I think "string style never worked" is actually a theory now not a hypothesis ;-), with the only counter data point in the tests / test fixtures here post-refactor 🤔 - though string certainly was documented, I just don't think it ever validated through joi before.

For these 4 consumers at least, we definitely need object style working again 🙏 - if I can help more in any way besides just banging my head against the wall of code ;-) just let me know

@thymikee thymikee changed the title fix(ios, scriptPhases): allow detailed scriptPhases object in config WIP fix(ios): allow detailed scriptPhases object in config May 20, 2022
Copy link
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

Thanks! This is a regression from refactoring linking configuration. Brought back scriptPhases to be an object for schema validation. No need to make it super explicit imho

@thymikee thymikee merged commit 9b9f681 into react-native-community:master May 20, 2022
@thymikee
Copy link
Member

Regarding react-native-firebase, it uses removed podspecPath config for iOS, which is discovered automatically. That's why it won't show in react-native config output. You should be good removing it from your custom config.

thymikee added a commit that referenced this pull request May 20, 2022
* fix(ios, scriptPhases): allow detailed scriptPhases object in config WIP

align the joi schema for scriptPhases with the actually allowed values

* fix: bring back object type to scripPhases validation

* fix: default to empty array

Co-authored-by: Michał Pierzchała <[email protected]>
@mikehardy mikehardy deleted the scriptPhases-object-allowed branch May 20, 2022 14:04
@mikehardy
Copy link
Contributor Author

Thanks a bunch @thymikee I really appreciate the help

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.

3 participants