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

lookup: add react #625

Merged
merged 5 commits into from
May 12, 2019
Merged

lookup: add react #625

merged 5 commits into from
May 12, 2019

Conversation

targos
Copy link
Member

@targos targos commented Nov 24, 2018

Hard Requirements:

  • Module source code must be on Github.
  • Published versions must include a tag on Github
  • The test process must be executable with only the commands npm install && npm test or (yarn install && yarn test) using the tarball downloaded from the Github tag mentioned above
  • The tests pass on supported major release lines (8 and 10)
  • The maintainers of the module remain responsive when there are problems
  • At least one module maintainer must be added to the lookup maintainers field

Soft Requirements:

  • The module must be actively used by the community OR
  • The module must be heavily depended on OR

Skip 6 and 11 because:

warn:                        | AssertionError [ERR_ASSERTION]: Current node version is not supported for development, expected "11.2.0" to satisfy "8.x || 9.x || 10.x".
warn:                        | at Object.<anonymous> (/tmp/30bf7968-ed35-442f-87ca-4c14c44c7bcf/react/node_modules/fbjs-scripts/node/check-dev-engines.js:39:3)

@codecov-io

This comment has been minimized.

@targos
Copy link
Member Author

targos commented Dec 7, 2018

/cc @acdlite @gaearon @sophiebits

CITGM is a tool that the Node.js project uses to detect if test suites of popular/important packages in the ecosystem are broken by changes. It is run before making new releases.

Is it OK if we add you as contacts to notify in case we find failures?
Additionally, would you accept a change in the react repo that allows us to run the tests with Node 11 and the master branch (i.e. bypass the check in check-dev-engines.js)?

@sophiebits
Copy link

Seems all right to me.

lib/lookup.json Outdated Show resolved Hide resolved
@targos
Copy link
Member Author

targos commented Dec 11, 2018

I just realized there was already a react entry, so I updated it and removed mine.

@targos
Copy link
Member Author

targos commented Dec 11, 2018

@SimenB SimenB mentioned this pull request Mar 12, 2019
@BridgeAR
Copy link
Member

BridgeAR commented Apr 3, 2019

@targos I guess this could land? I do not remember the CI outcome but otherwise this should be fine.

@BridgeAR
Copy link
Member

Ping @targos

@targos
Copy link
Member Author

targos commented May 11, 2019

@BridgeAR
Copy link
Member

BridgeAR commented May 11, 2019

The AIX issue is an issue with yarn, if I remember correct. Should we automatically skip AIX in case yarn is used? That would require a code change and we should probably run them and fail in case AIX actually passes to notify us when it's time to remove that special handling again. I am not really sold on it being ideal but I thought I suggest that nevertheless to hear what you think.

Update: #625 (comment)

@targos
Copy link
Member Author

targos commented May 11, 2019

The AIX issue is #688

@targos
Copy link
Member Author

targos commented May 11, 2019

I think the windows failure is infra-related (I saw the same error with another run today).
I suggest we merge this like this and iterate.

@richardlau
Copy link
Member

The AIX issue is an issue with yarn, if I remember correct. Should we automatically skip AIX in case yarn is used? That would require a code change and we should probably run them and fail in case AIX actually passes to notify us when it's time to remove that special handling again. I am not really sold on it being ideal but I thought I suggest that nevertheless to hear what you think.

It will only pass on AIX when we actually get the minimum supported version of AIX (7.1) in our CI (we're still on 6.3 😞). cc @nodejs/platform-aix

@BridgeAR
Copy link
Member

@richardlau thanks for the heads up!

@targos targos merged commit 361f600 into master May 12, 2019
@targos targos deleted the react branch May 12, 2019 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants