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

Sort selections in a stable sort order #2681

Closed
wants to merge 6 commits into from

Conversation

Projects
None yet
4 participants
@eliperkins
Copy link
Contributor

commented Mar 7, 2019

This fixes an issue that was raised in Node 11, where sort may call to items in a different order than other versions of Node.

Fixes #2675

Enable Node 11 in CI
This will highlight the issue posed on #2675 and nodejs/node#24294
"__typename": "Comment",
"body": {
"text": "<mock-value-for-field-\\"text\\">"
"__typename": "User",

This comment has been minimized.

Copy link
@eliperkins

eliperkins Mar 7, 2019

Author Contributor

This looks like an odd change... 🤔

This comment has been minimized.

Copy link
@josephsavona

josephsavona Mar 7, 2019

Contributor

I think what’s happening is that we’re able to skip more redundant nodes as a result of this change (exactly what I’d expect). I need to look at this more and try it locally to confirm.

Show resolved Hide resolved packages/relay-compiler/transforms/SkipRedundantNodesTransform.js
@@ -745,12 +750,16 @@ fragment TestFragment on Viewer {
{
"variables": {},
"payload": {
"__typename": "Page",

This comment has been minimized.

Copy link
@eliperkins

eliperkins Mar 7, 2019

Author Contributor

This too. 🤔 🤔 🤔

},
[[], []],
);
return [...scalarsAndLinkedFields, ...rest];

This comment has been minimized.

Copy link
@josephsavona

josephsavona Mar 7, 2019

Contributor

Repeatedly creating new arrays like this is incredibly inefficient. Instead, let’s create just two new arrays and partition the elements into them. There’s a helper function partitionArray() (or similar) in RelayParser that we can extract into a module and reuse it for this.

This comment has been minimized.

Copy link
@eliperkins

eliperkins Mar 7, 2019

Author Contributor

Done in 516ae8c

Show resolved Hide resolved packages/relay-compiler/transforms/SkipRedundantNodesTransform.js
"__typename": "Comment",
"body": {
"text": "<mock-value-for-field-\\"text\\">"
"__typename": "User",

This comment has been minimized.

Copy link
@josephsavona

josephsavona Mar 7, 2019

Contributor

I think what’s happening is that we’re able to skip more redundant nodes as a result of this change (exactly what I’d expect). I need to look at this more and try it locally to confirm.

@eliperkins eliperkins changed the title [WIP] Sort selections in a stable sort order Sort selections in a stable sort order Mar 7, 2019

@facebook-github-bot
Copy link

left a comment

@kassens has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@kassens

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

This looks good to me. Now testing internally. The only update I made was adding the file headers and keeping the old sort behind a Rollout flag since this introduces so many changes we have to land them in multiple waves internally 😂

@eliperkins eliperkins deleted the eliperkins:fix-2675 branch Mar 11, 2019

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.