React-Native 0.65 compatibility / Forward-port Accessibility to react-native 0.60 API (#1125) #1126
Conversation
|
I don't believe the Microsoft.reactxp CI step will pass until the related DefinitelyTyped PR is resolved |
|
Incidentally, how are people using RXPTest during this RN60 transition? Should I port it to RN60? Has someone already done that? I want to run through this feature there but I don't want to duplicate effort |
| this._updateScreenReaderStatus(isEnabled); | ||
| }); | ||
|
|
||
| // Fetch initial state. | ||
| RN.AccessibilityInfo.fetch().then(isEnabled => { | ||
| if (!initialStateChanged) { | ||
| RN.AccessibilityInfo.isScreenReaderEnabled().then(isEnabled => { |
There was a problem hiding this comment.
This will crash on non-0.60 versions of RN - fetch currently points at isScreenReaderEnabled - we should probable leave it like that
There was a problem hiding this comment.
That is true, I propose this in the context of a reactxp 2.0 release which will necessarily (I think?) be a breaking change release that is only compatible with >= RN0.60
If I am wrong in that, then I agree with leaving it, but if it cuts off <RN60 then might as well move now? (also assuming my DefinitelyTyped changes are accepted)
There was a problem hiding this comment.
Reconsidering - I just read through the code (previously I just worked off the docs which make no mention of fetch continuing to exist) and both the old fetch() API call and the "change" event name do exist (thanks for that clue), as such I think two things now:
- this is not blocking reactxp compatibility for RN60 at all, so there is no pressure here, we can get it right (as in, cover the full change)
- I should update my PR to DefinitelyTyped such that it contains the deprecated functions still (I will do this)
I still think it's not vital to cover RN<0.60 for reactxp@^2 but that is only because of the assumption reactxp@^2 will have incompatible changes in other areas.
That's worth considering + verifying because it would instruct when to land this PR - either immediately when it's in good shape or later when RN0.60 was a reasonable lower-bound
Either way, thanks for the review - it's will directly improve my DefinitelyTyped PR since I missed that the fetch API existed still
There was a problem hiding this comment.
Last I chatted with Eric, he wanted to keep the RN compatibility backwards compatible for ~1yr of releases where feasible. Currently there's nothing in 0.60 that constitutes a full breaking change to reactxp, so ideally we'd leave this with the fetch implementation. Is there anything added to new implementation besides the name change?
There was a problem hiding this comment.
Ok - I prefer to be backwards-compatible (aka user friendly) when possible so that works for me. I had a misunderstanding about reactxp@^2 then, and good that it's corrected.
There is more to the API change, there are new features exposed - check the @types diff here https://github.com/DefinitelyTyped/DefinitelyTyped/pull/37745/files
I am not sure the best way to do it but I think it would be cool to expose all the Accessibility getters/events if possible so the plumbing is there for people focused on making accessible apps.
My hesitation is that it was 2 item (screen reader + announce) but now there are 8 items, and of those: 7 work on iOS (all but high contrast), 2 work on Android (screen reader + announce), 3 work on windows (screen reader + announce, and high contrast), then for web it looks like announce only? It is very divergent. But I think the implementation path would be plumb the types through the common then native-common Accessibility with defaults to false similar to high-contrast, and override in iOS where they have the extra features? Then get those in a backwards-compatible way in iOS implementation?
If there's no appetite for fully covering the API then just as a breaking name change I think this is pointless and we should just wait for July 2020 (~1year) then do it. But at least the typings are ready ;-)
There was a problem hiding this comment.
Typically if there's new APIs available in RN that we want to expose, we check if the function exists before calling, if it doesn't, return a sensible default value. This allows us to still provide back-compat while exposing features to consumers of newer RN. This is inline with your suggestion for exposing the new iOS functionality but adds a little more protection.
@erictraut - do you have strong opinions about exposing new RN accessibility features that may not be implemented on all RN platforms? My other thought is that this could be exposed as a separate reactxp extension until such a time where everything is supported in a consistent manner
|
still waiting on the related PR that provides the types, FWIW - DefinitelyTyped/DefinitelyTyped#37745 |
|
My @types/react-native PR with the new Accessibility API landed, so this should be unblocked now. |
|
@berickson1 this is ready for review again I think? Pending resolution on your requests earlier it may be good to go - if not I'll standby to shape it. Thanks! |
|
Yesterday on 0.62, I ran into the need for this PR. (#1187) — we are leapfrogging from 0.59 to RN 0.62 which is at RC3 The I’d like to raise the priority on this fix, as April 30th Apple is requiring iOS 13 SDK, which more or less requires Xcode 11, and 0.60, 0.61, and 0.62 all had lots of changes to improve compatibility with Xcode 11. I’m thrilled that this PR exists @mikehardy! Thanks! |
|
Looking at the comments, I think the sticking point is that we prefer backwards compatibility to as far back as we can. I think where we’re unclear is exactly what strategy needs to be applied here to either get this PR in alignment, or to make a new PR? Do @berickson1, @erictraut, or @mikehardy have a feeling of what kind of PR would be accepted here? I and a colleague have some time to contribute here, as we would like our release to go into testing in the next two weeks. For my purposes, we weren’t directly using this part of the Accessibility api in our app — yet. Come August, I believe we’ll start to want access to RN’s newly added APIs. Please let me how I can help move us forwards, thanks! |
|
Just a note that .62 is on RC5 now and indications are that it's down to "this might be it..." status, as in it's about to go stable / general release literally any day. I also stand ready to make sure this works. It should be noted that react-native 0.61 does work just fine with Xcode 11 / iOS13, I've been using it for quite some time. Although projects may have different edge cases Finally I will say that this PR is usable via patch-package once you patch it in to node_modules manually as long as you call the reactxp build in postinstall after patch-package to get the change in. I've done that with other patches in the past on reactxp |
|
@mikehardy -- we're on they've reached RN 0.63.2 now, so… maybe we should figure out what it takes to get this merged :P |
|
@fbartho I think this comment from the in-line review is the most prescriptive #1126 (comment) for exposing new APIs or re-shaping this if needed |
|
@mikehardy -- would you mind resolving the package.json conflicts? Additionally let's figure out what remaining action items there are to get this PR merged & what other PRs/Features need to fast-follow! |
|
Sure thing - I'll put this in my list in the high priority section but that's still an ETA of a day or three |
|
Not at all that urgent from my side / I have plenty of competing challenges. I've tentatively drawn a line in the sand to have a release ready by September 1st -- but that's just a date to help focus my efforts. Over the next two weeks so, I want to do a dive through all the backlog of Issues, and try to build a Roadmap of some kind + respond to all open tickets, and/or close any dead ones. |
|
perfect, I work the same way (deadlines to focus, but it's open source after all) and with full sweeps when assuming maintainership of something. I had good luck with installing the welcome bot (not the default github first issue / first PR bot as PRs have a permission issue from forks) and the "stale bot" on my biggest maintained repo for what it's worth: welcome bot: ankidroid/Anki-Android@5e0086e stale bot: https://github.com/ankidroid/Anki-Android/blob/master/.github/workflows/stale.yml |
|
@fbartho rebased + re-pushed |
It's been more than a year, and react-native 0.65 just dropped AccessibilityInfo.fetch so reactxp is officially not working with current react-native until this is merged |
|
Here's the patch-package format patch if anyone else is trying to make all this stuff work... @fbartho / @berickson1 would be great to see a release with this - everything appears to work on RN65 after this patch. |
mikehardy
changed the title
|
So, a couple years after originally posting this, and a few months after posting that it was a hard breaking change in react-native compatibility, it's pretty obvious this package is dead. It's important to recognize that and heed the message of the inactivity. years after both projects were just beginning, it's clear react-native-web has won the community mindshare here. I've de-integrated reactxp and ported to react-native-web. Works pretty well these days, and not a bad path forward. I'm closing this, cheers all! |
React-Native 0.59 -> 0.60 changed a lot of the AccessibilityInfo API, and this does a straight forward-port of existing functionality (screen reader status detection) to the new API.
Here is the related PR facebook/react-native-website#835
And the doc with new APIs https://facebook.github.io/react-native/docs/accessibilityinfo
This depends on the PR here for types I believe DefinitelyTyped/DefinitelyTyped#37486
There is an opportunity to also expand API coverage to cover the other new accessibility features react-native exposes but if this is desired it might be best to do it separately? Additionally the majority are iOS-specific so if I proposed that change I'd need some guidance.
Fixes #1319