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

Rename ReactPerf methods to match the upcoming ReactPerf revamp #6287

Merged
merged 1 commit into from Mar 17, 2016

Conversation

Projects
None yet
3 participants
@gaearon
Copy link
Member

commented Mar 17, 2016

I’m not feeling strong about this but I would like to get these two methods renamed before 15.0.

printDOM()printOperations()

printDOM() is really popular one but since ReactPerf can be used together with React Native, the name doesn’t really do it justice. In #6046, we let renderers emit arbitrary NATIVE_OPERATION events so React Native won’t be confined to the current list of “DOM operations” either. This is why I’m renaming printDOM() to printOperations().

getMeasurementsSummaryMap()getWasted()

The second rename concerns getMeasurementsSummaryMap() which was added in #2219. Its name is already somewhat confusing because it doesn’t actually return a map—it returns an array. Additionally, it sounds very generic but it actually corresponds to a very specific dataset: the table printed by printWasted().

In #6046 I plan to expose a get* method for every print* method so it makes sense that getWasted() exists as a match to printWasted() in 15.0, complemented by other get*() methods such as getInclusive() and getOperations() when #6046 ships some time during 15.x.

One thing I’m not sure about is whether getWasted() invites an unnecessary connotation for fluent English speakers. I’m open to improvements that offer another way of maintaining consistency with the upcoming changes, e.g. getWastedStats(), getInclusiveStats(), etc.

Why Do This Now

This is our opportunity to rename some methods on the API that’s going to significantly change internally, and I think it’s worth doing it before shipping 15.0. Since people rely on getMeasurementsSummaryMap() for things like CI (#2219), in my opinion it would be unwise to introduce a deprecation warning in something other than a major release, as a new warning can potentially break CIs.

Deprecation Strategy

We will keep the old method name working in v15 with a deprecation warning, and remove it during or after v16 when most tutorials have been updated.

Reviewers

@sebmarkbage @jimfb

@gaearon gaearon added this to the 15.0 milestone Mar 17, 2016

@facebook-github-bot

This comment has been minimized.

Copy link

commented Mar 17, 2016

@gaearon updated the pull request.

@sebmarkbage

This comment has been minimized.

Copy link
Member

commented Mar 17, 2016

Lgtm. I wouldn't worry too much about deprecation strategy for these. You can also just break them next release. Their use in code is very limited. Often in non-critical tools. They're also isolated to small projects and easy to find.

@gaearon

This comment has been minimized.

Copy link
Member Author

commented Mar 17, 2016

Tutorials reference printDOM() very often so I want to keep it for a while so people don’t think we killed it or something.

As for getMeasurementsSummaryMap() it wasn’t even documented but I figured that since we deprecate one with a warning, might as well do that for the other one.

gaearon added a commit that referenced this pull request Mar 17, 2016

Merge pull request #6287 from gaearon/rename-perf-methods
Rename ReactPerf methods to match the upcoming ReactPerf revamp

@gaearon gaearon merged commit 5520c39 into facebook:master Mar 17, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@gaearon gaearon deleted the gaearon:rename-perf-methods branch Mar 17, 2016

@renovate renovate bot referenced this pull request Feb 2, 2018

Open

Update dependency react to v0.14.9 #29

0 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.