This repository has been archived by the owner on Jun 17, 2022. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 404
docs(CHANGELOG.md): Document ScrollView ES6 conversion #77
Merged
turnrye
merged 2 commits into
react-native-community:0.58-changelog
from
RSNara:scrollview-change
Jan 9, 2019
Merged
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
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.
Should this be in a separate Breaking Changes section? I imagine it will be something people will need to be specifically aware of in their upgrades.
Also, perhaps we can provide more information on the specific methods that have changed and an example of how their code will need to be changed.
Is there anything we can do to help people automatically detect these old calls in their codebase proactively without waiting for something to crash?
Really I just want to help people upgrade as easily as possible, providing any information / tools that helps make that happen.
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 it's important that this description is flipped; the conversion to ES6 explains why the change took place, but the important thing to users is understanding that ScrollView's public methods are no longer instance bound. I'd suggest leading with that and continuing with the ES6 conversion bit as explanation. I think to @TheSavior's point, this becomes a BigDeal™ and should probably come before the Flowtypes mention.
Thanks for helping with the 58 release notes! Swap this around and I'll happily merge it into my branch.
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 it's a good idea to have a breaking changes section. My only worry is that it'll never be all-encompassing/complete, unless we make an active effort to document all breaking changes to the codebase, which won't be trivial. (And even if we do make this effort, we'll always miss things). Also, unless we explicitly document that this section is not complete (which we could totally do), some people will assume that it is complete.
I thought about this and I think it may be more succinct to say that all methods you can invoke on the component will no longer be bound to the component instance. But... I think it might be a good idea to list out the methods incase someone is CTRL+F'ing through the changelog.
I'm not sure if there's anything we can do, but the easiest way of finding these problems that I can think of would be to grep through the codebase looking for cases where the functions are passed in as callbacks to methods. So, looking for strings like this: 'flashScrollIndicators)' or 'flashScrollIndicators,'. I'm not sure what else we can do aside from being explicit about which functions were affected, which would help people change up these call-sites.