Skip to content
This repository has been archived by the owner on Dec 30, 2022. It is now read-only.

chore(deps): update Node to v12 #2922

Merged
merged 12 commits into from
May 13, 2020
Merged

chore(deps): update Node to v12 #2922

merged 12 commits into from
May 13, 2020

Conversation

eunjae-lee
Copy link
Contributor

@eunjae-lee eunjae-lee commented May 4, 2020

Summary

  1. examples/react-native and examples/react-native-query-suggestions has been re-written. I couldn't update their dependencies correctly to make them work. So I created new projects from scratch by using expo, and I copied&pasted the same code we had there.

  2. The only part I changed in the examples is, replacing ListView to FlatList. ListView is deprecated, so I had to do that.

  3. Except for the examples, there isn't much. Let me know if something isn't clear.

And let me know if you wish me to tidyng up this PR by creating stacked PRs for those examples.

@algobot
Copy link
Contributor

algobot commented May 4, 2020

Deploy preview for react-instantsearch ready!

Built with commit 28d4380

https://deploy-preview-2922--react-instantsearch.netlify.app

@eunjae-lee
Copy link
Contributor Author

eunjae-lee commented May 4, 2020

There was one part that caused many failing tests.
I haven't digged enough but, I guess it has something to do with the change of implementation of Array.sort().

nodejs/node#24294

@eunjae-lee
Copy link
Contributor Author

eunjae-lee commented May 5, 2020

I didn't manage to run react-native and react-native-query-suggestions examples with Node v12.
Many dependencies were outdated. After many trials, I just deleted the examples and re-created them with expo init <project-name> and copy&paste the contents into the new examples.
This way, we kind of lose the history in the examples, but there aren't much history to track, IMO.

Plus, I've replaced deprecated ListView usages in the examples with FlatList which is new.

This will close #2908

@eunjae-lee
Copy link
Contributor Author

The only remaining part in this PR is to make test:integration work.
For some reason, it's failing.
So I checked out master branch on my computer and it still does not work.

Does someone, probably @yannickcr, know if there's any special config to run this locally on master branch?

Screenshots in progress...
(node:6193) UnhandledPromiseRejectionWarning: Error: net::ERR_EMPTY_RESPONSE at http://localhost:8080/iframe.html?id=integration-with-other-libraries--airbnb-rheostat
    at navigate (/Users/eunjaelee/workspace/react-instantsearch/node_modules/puppeteer/lib/FrameManager.js:120:37)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:189:7)
  -- ASYNC --
    at Frame.<anonymous> (/Users/eunjaelee/workspace/react-instantsearch/node_modules/puppeteer/lib/helper.js:111:15)
    at Page.goto (/Users/eunjaelee/workspace/react-instantsearch/node_modules/puppeteer/lib/Page.js:674:49)
    at Page.<anonymous> (/Users/eunjaelee/workspace/react-instantsearch/node_modules/puppeteer/lib/helper.js:112:23)
    at takeScreenshot (/Users/eunjaelee/workspace/react-instantsearch/integration/takeScreenshot.js:14:14)
    at /Users/eunjaelee/workspace/react-instantsearch/integration/runTest.js:46:11
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:189:7)
(node:6193) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:6193) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

@eunjae-lee eunjae-lee marked this pull request as ready for review May 11, 2020 16:08
@eunjae-lee eunjae-lee requested a review from a team May 11, 2020 16:08
@ghost ghost requested review from Haroenv and yannickcr and removed request for a team May 11, 2020 16:08
Copy link
Contributor

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven’t tried the rn example yet

@@ -23,7 +23,7 @@ aliases:
defaults: &defaults
working_directory: ~/react-instantsearch
docker:
- image: circleci/node:8.16.1@sha256:9e83d4b3b046d1c7591952865bc4d803d2ddeaf1c6352f76c2133bbe96606d40
- image: circleci/node:12.14.1@sha256:bc28f8653824cc4e28902e73d140011c94dfeda8b1f83393d049daa468f51693
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This no longer has -browsers. Expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my trials, only the integration test requires -browser image.

const isFirstWidgetIndex = isIndexWidget(firstWidget);
const isSecondWidgetIndex = isIndexWidget(secondWidget);

if (isFirstWidgetIndex && !isSecondWidgetIndex) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you need to make these changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding that function, lots of tests failed. I searched for it and there has been a change in the implementation of Array.sort() in Node v11 (link).
And the previous implementation of sortIndexWidgetsFirst didn't consider the both widgets being index. After fixing them like this, all the tests passed.

.circleci/config.yml Outdated Show resolved Hide resolved
Co-authored-by: Yannick Croissant <[email protected]>
@eunjae-lee eunjae-lee merged commit 786b9e8 into master May 13, 2020
@eunjae-lee eunjae-lee deleted the chore/update-node-12 branch May 13, 2020 08:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants