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

Feat new react-play: GitHub User Search #309

Merged
merged 35 commits into from Jun 28, 2022
Merged

Conversation

TejasShekar
Copy link
Contributor

@TejasShekar TejasShekar commented Jun 14, 2022

First thing, PLEASE READ THIS: ReactPlay Code Review Checklist

Description

Added a new react-play for beginner ideas. Play name is GitHub User Search

  • Used basic hooks like useState and useEffect for managing data and error handling
  • Used Axios for data fetching
  • Added media queries for responsiveness
  • Added rate limit info.
  • Kept it as simple as possible

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

I have not tested my feature as I am unaware of the testing procedures at the moment.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@vercel
Copy link

vercel bot commented Jun 14, 2022

@TejasShekar is attempting to deploy a commit to a Personal Account owned by @reactplay on Vercel.

@reactplay first needs to authorize it.

@vercel
Copy link

vercel bot commented Jun 14, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
react-play ✅ Ready (Inspect) Visit Preview Jun 27, 2022 at 5:18AM (UTC)

@atapas
Copy link
Member

atapas commented Jun 14, 2022

The build fails with this error:

image

@atapas atapas linked an issue Jun 14, 2022 that may be closed by this pull request
@atapas
Copy link
Member

atapas commented Jun 14, 2022

The build fails with this error:

image

@TejasShekar You may want to run yarn build locally and see if you get a similar error there. It happens usually when we make a call that takes time to fetch data. Kindly do some research and proceed.

@TejasShekar
Copy link
Contributor Author

TejasShekar commented Jun 14, 2022

The build fails with this error:
image

@TejasShekar You may want to run yarn build locally and see if you get a similar error there. It happens usually when we make a call that takes time to fetch data. Kindly do some research and proceed.

Neither yarn build nor npm run build seems to have issue on my local system. The build does complete and I am able to run the build folder on my localhost via live server.
But I would like to inform you about the two errors that I have been consistent when I run the build commands i.e. either yarn or npm,

Error No. 1 :

🔥  error at /plays/dynamic-routes/:menu 
Error: EINVAL: invalid argument, mkdir 'D:\Own Projects\react-play\build\plays\dynamic-routes\:menu'
    at Object.mkdirSync (node:fs:1334:3)
    at Function.sync (D:\Own Projects\react-play\node_modules\react-snap\node_modules\mkdirp\index.js:71:13)
    at saveAsHtml (D:\Own Projects\react-play\node_modules\react-snap\index.js:565:12)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async afterFetch (D:\Own Projects\react-play\node_modules\react-snap\index.js:776:9)
    at async fetchPage (D:\Own Projects\react-play\node_modules\react-snap\src\puppeteer_utils.js:244:24) {
  errno: -4071,
  syscall: 'mkdir',
  code: 'EINVAL',
  path: 'D:\\Own Projects\\react-play\\build\\plays\\dynamic-routes\\:menu'
}

Error No. 2

🔥  pageerror at /plays/simple-live-chat: FirebaseError: Firebase: Error (auth/invalid-api-key).
    at Gc (../node_modules/@firebase/auth/src/core/util/assert.ts:128:9)
    at Kc (../node_modules/@firebase/auth/src/core/util/assert.ts:153:29)
    at _assert (../node_modules/@firebase/auth/src/core/auth/register.ts:67:10)
    at app (../node_modules/@firebase/auth/src/core/auth/register.ts:90:11)
    at instanceFactory (../node_modules/@firebase/component/src/provider.ts:318:32)
    at getOrInitializeService (../node_modules/@firebase/component/src/provider.ts:242:26)
    at initialize (../node_modules/@firebase/auth/src/core/auth/initialize.ts:66:24)
    at initializeAuth (../node_modules/@firebase/auth/src/platform_browser/index.ts:44:9)
    at getAuth (plays/simple-live-chat/components/signin-button.jsx:23:15)

The only message related to my submitted play that I saw was
onsole.log at /plays: Cover image not found for the play GitHub User Search

@TejasShekar TejasShekar requested a review from atapas June 14, 2022 13:09
@atapas
Copy link
Member

atapas commented Jun 14, 2022

The build fails with this error:
image

@TejasShekar You may want to run yarn build locally and see if you get a similar error there. It happens usually when we make a call that takes time to fetch data. Kindly do some research and proceed.

Neither yarn build nor npm run build seems to have issue on my local system. The build does complete and I am able to run the build folder on my localhost via live server. But I would like to inform you about the two errors that I have been consistent when I run the build commands i.e. either yarn or npm,

Error No. 1 :

🔥  error at /plays/dynamic-routes/:menu 
Error: EINVAL: invalid argument, mkdir 'D:\Own Projects\react-play\build\plays\dynamic-routes\:menu'
    at Object.mkdirSync (node:fs:1334:3)
    at Function.sync (D:\Own Projects\react-play\node_modules\react-snap\node_modules\mkdirp\index.js:71:13)
    at saveAsHtml (D:\Own Projects\react-play\node_modules\react-snap\index.js:565:12)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async afterFetch (D:\Own Projects\react-play\node_modules\react-snap\index.js:776:9)
    at async fetchPage (D:\Own Projects\react-play\node_modules\react-snap\src\puppeteer_utils.js:244:24) {
  errno: -4071,
  syscall: 'mkdir',
  code: 'EINVAL',
  path: 'D:\\Own Projects\\react-play\\build\\plays\\dynamic-routes\\:menu'
}

Error No. 2

🔥  pageerror at /plays/simple-live-chat: FirebaseError: Firebase: Error (auth/invalid-api-key).
    at Gc (../node_modules/@firebase/auth/src/core/util/assert.ts:128:9)
    at Kc (../node_modules/@firebase/auth/src/core/util/assert.ts:153:29)
    at _assert (../node_modules/@firebase/auth/src/core/auth/register.ts:67:10)
    at app (../node_modules/@firebase/auth/src/core/auth/register.ts:90:11)
    at instanceFactory (../node_modules/@firebase/component/src/provider.ts:318:32)
    at getOrInitializeService (../node_modules/@firebase/component/src/provider.ts:242:26)
    at initialize (../node_modules/@firebase/auth/src/core/auth/initialize.ts:66:24)
    at initializeAuth (../node_modules/@firebase/auth/src/platform_browser/index.ts:44:9)
    at getAuth (plays/simple-live-chat/components/signin-button.jsx:23:15)

The only message related to my submitted play that I saw was onsole.log at /plays: Cover image not found for the play GitHub User Search

@erayalkis I was wondering if you know about the Timeout error we see on preview build.

@TejasShekar Ignore the other two. They are harmless.

@atapas atapas requested a review from erayalkis June 14, 2022 13:16
@TejasShekar
Copy link
Contributor Author

@atapas You might want to look into this. Seems like an issue with puppeteer.
https://ourcodeworld.com/articles/read/1106/how-to-solve-puppeteer-timeouterror-navigation-timeout-of-30000-ms-exceeded

@atapas
Copy link
Member

atapas commented Jun 14, 2022

@atapas You might want to look into this. Seems like an issue with puppeteer. https://ourcodeworld.com/articles/read/1106/how-to-solve-puppeteer-timeouterror-navigation-timeout-of-30000-ms-exceeded

We use Puppeteer from the react-snap. Do you want to check from that side? It is in package.json file. May be we need to set some flag there.

For this reference: puppeteer/puppeteer#782 (comment)

If you want to check it out, can do changes in your branch and see if that fixes the issue. I'll also check it out tomorrow.

@TejasShekar

@erayalkis
Copy link
Collaborator

Hi @atapas! I'd love to help out! 😄
I've looked into this error a while ago and it seems like the only available options were either downgrading react-snap or puppeteer. I believe we downgraded puppeteer a while ago, so I'm going to assume that didn't work. We could maybe try downgrading react-snap?

@atapas
Copy link
Member

atapas commented Jun 15, 2022

Hi @atapas! I'd love to help out! 😄
I've looked into this error a while ago and it seems like the only available options were either downgrading react-snap or puppeteer. I believe we downgraded puppeteer a while ago, so I'm going to assume that didn't work. We could maybe try downgrading react-snap?

Thanks! Give it a shot. You can use this same branch where the error is reproduced everytime.

@TejasShekar
Copy link
Contributor Author

TejasShekar commented Jun 17, 2022

I have been trying to find out the error due to which the build is failing on vercel. My recent commits before the deployment started failing was just merging the commits from upstream repo to my origin repo and remove the empty stylesheet and its import.
yarn build now fails locally as well only when the firebase: invalid key error comes from the live chat play. I comment out the related code in meta.js and run the build command and it is successful.
Is that issue not fixable ? @atapas

@atapas
Copy link
Member

atapas commented Jun 17, 2022

I have been trying to find out the error due to which the build is failing on vercel. My recent commits before the deployment started failing was just merging the commits from upstream repo to my origin repo and remove the empty stylesheet and its import. yarn build now fails locally as well only when the firebase: invalid key error comes from the live chat play. I comment out the related code in meta.js and run the build command and it is successful. Is that issue not fixable ? @atapas

@TejasShekar It is not an issue with your code. I think it is the problem with vercel. I'll look into it.

Regarding build failing locally, you need create an .env file and add following keys to it with valid firebase values. You can get these from your firebase profiles.

REACT_APP_FIREBASE_API_KEY,
REACT_APP_FIREBASE_AUTH_DOMAIN,
REACT_APP_FIREBASE_DATABASE_URL,
REACT_APP_FIREBASE_PROJECT_ID,
REACT_APP_FIREBASE_STORAGE_BUCKET,
REACT_APP_MESSAGING_SENDERID,
REACT_APP_FIREBASE_APP_ID,

@TejasShekar
Copy link
Contributor Author

I have been trying to find out the error due to which the build is failing on vercel. My recent commits before the deployment started failing was just merging the commits from upstream repo to my origin repo and remove the empty stylesheet and its import. yarn build now fails locally as well only when the firebase: invalid key error comes from the live chat play. I comment out the related code in meta.js and run the build command and it is successful. Is that issue not fixable ? @atapas

@TejasShekar It is not an issue with your code. I think it is the problem with vercel. I'll look into it.

Regarding build failing locally, you need create an .env file and add following keys to it with valid firebase values. You can get these from your firebase profiles.

REACT_APP_FIREBASE_API_KEY,
REACT_APP_FIREBASE_AUTH_DOMAIN,
REACT_APP_FIREBASE_DATABASE_URL,
REACT_APP_FIREBASE_PROJECT_ID,
REACT_APP_FIREBASE_STORAGE_BUCKET,
REACT_APP_MESSAGING_SENDERID,
REACT_APP_FIREBASE_APP_ID,

Yes sir, I understood I don't have the .env file as it was not me who built the chat app and we should never push them to GitHub and only include them in deployment tools like vercel, netlify so that build does not fail.

@atapas
Copy link
Member

atapas commented Jun 19, 2022

@TejasShekar Please pull the branch from main.. the React snap issue is avoided for the preview build.

- added type attr for form button
- prettier did its work on the other file.
atapas
atapas previously approved these changes Jun 22, 2022
Copy link
Member

@atapas atapas left a comment

Choose a reason for hiding this comment

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

My Review is done, looks good.

@koustov could you please finish too?

@atapas
Copy link
Member

atapas commented Jun 26, 2022

Hi @TejasShekar

We have some massive changes in the data store and we are changing how the play gets created for a better usability. As this play is interim, we need to make sure it migrates correctly. Could you please ping me on Discord so that I make sure everything is placed?

https://discord.gg/YbmXwU5f

@TejasShekar
Copy link
Contributor Author

Hi @TejasShekar

We have some massive changes in the data store and we are changing how the play gets created for a better usability. As this play is interim, we need to make sure it migrates correctly. Could you please ping me on Discord so that I make sure everything is placed?

https://discord.gg/YbmXwU5f

Sure, sir.

@TejasShekar
Copy link
Contributor Author

@atapas All changes as requested has been made. Awaiting reviews and merging.

Copy link
Member

@koustov koustov left a comment

Choose a reason for hiding this comment

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

lgtm

@koustov koustov merged commit 9a47972 into reactplay:main Jun 28, 2022
@atapas
Copy link
Member

atapas commented Jun 28, 2022

@all-contributors please add @TejasShekar for Code

@allcontributors
Copy link
Contributor

@atapas

I've put up a pull request to add @TejasShekar! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Add a Play]: GitHub User Search
4 participants