-
Notifications
You must be signed in to change notification settings - Fork 492
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
React-Native 0.65 compatibility / Forward-port Accessibility to react-native 0.60 API (#1125) #1126
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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