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

Fix(@rjsf/antd): Check for errors.length #2576

Merged

Conversation

ChocolateLoverRaj
Copy link
Contributor

Reasons for making this change

Fixes #2575

Checklist

  • I'm updating documentation
  • I'm adding or updating code
    • I've added and/or updated tests
    • I've updated docs if needed
    • I've updated the changelog with a description of the PR
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

@ChocolateLoverRaj
Copy link
Contributor Author

It's disappointing that after almost a year my bug fix didn't get merged and now the build is failing so I'm guessing it's even less likely to get merged.

@heath-freenome
Copy link
Member

heath-freenome commented Aug 29, 2022

@ChocolateLoverRaj Sorry for the slow response... Up until a few months ago this project was running on a skeleton crew (i.e. one person in grad school). If you'd be willing to fix the conflict and the tests, I'd be happy to review and merge this into the v5 beta

@ChocolateLoverRaj
Copy link
Contributor Author

Ok i will fix conflict

CHANGELOG.md Outdated
Comment on lines 275 to 279
# v3.1.1 (upcoming)

## @rjsf/antd
- Do not show errors if `extraErrors` has `[]` (https://github.com/rjsf-team/react-jsonschema-form/pull/2576).

Copy link
Member

Choose a reason for hiding this comment

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

Can you move this up to the 5.0.0-beta.3 section?

@@ -0,0 +1,52 @@
import React from 'react';
Copy link
Member

Choose a reason for hiding this comment

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

Would you be willing to add this to the Array.test.js instead, at the end?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

import React from 'react'

is already in Array.test.js

Copy link
Member

@heath-freenome heath-freenome Aug 29, 2022

Choose a reason for hiding this comment

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

I'm not sure you understand what I mean... You created a new EmptyErrorsArray.test.js file... Can you instead, copy the two tests and paste them at around line 77 in the Array.test.js file? Also, you'll need to now pass the required validator={validator} parameter into the Form

@heath-freenome
Copy link
Member

Also, we are strictly checking formatting now, so you'll need to do npm run cs-format before you commit... Or you can setup husky to do this automatically on commit by doing npm run prepare at the root directory after doing npm install

@ChocolateLoverRaj
Copy link
Contributor Author

@heath-freenome npm i and pnpm i both don't work. When lerna bootstrap does work, the tests fail because they can't import the dependencies. I can't work on this GH repo because of that.

@heath-freenome
Copy link
Member

heath-freenome commented Aug 30, 2022

@heath-freenome npm i and pnpm i both don't work. When lerna bootstrap does work, the tests fail because they can't import the dependencies. I can't work on this GH repo because of that.

That is odd... What error(s) are you seeing? Can you show me screen shots?

@ChocolateLoverRaj
Copy link
Contributor Author

ChocolateLoverRaj commented Aug 30, 2022

This is the error I get (on master branch, so it's not specific to my branch / changes):

$ pnpm i
 WARN  deprecated source-map-resolve@0.6.0: See https://github.com/lydell/source-map-resolve#deprecated
Packages: +4 -910
+------------------------------------------------------------------------------------------------------------------------------------------------------------  
Progress: resolved 2697, reused 1145, downloaded 0, added 0, done

> react-jsonschema-form@5.0.0 postinstall C:\Users\rajas\Documents\GitHub\react-jsonschema-form
> lerna bootstrap

lerna notice cli v5.4.3
lerna info Bootstrapping 11 packages
lerna info Installing external dependencies
lerna info Symlinking packages and binaries
lerna WARN EREPLACE_EXIST @rjsf/core is already installed for @rjsf/semantic-ui. Replacing with symlink...
lerna WARN EREPLACE_EXIST @rjsf/utils is already installed for @rjsf/semantic-ui. Replacing with symlink...
lerna success Bootstrapped 11 packages

> react-jsonschema-form@5.0.0 prepare C:\Users\rajas\Documents\GitHub\react-jsonschema-form
> husky install

husky - Git hooks installed

dependencies:
- @ant-design/icons 4.7.0
- @babel/cli 7.18.10
- @babel/core 7.18.13
- @babel/plugin-proposal-class-properties 7.18.6  
- @babel/plugin-proposal-object-rest-spread 7.18.9
- @babel/plugin-proposal-optional-chaining 7.18.9 
- @babel/plugin-syntax-dynamic-import 7.8.3       
- @babel/plugin-transform-modules-commonjs 7.18.6 
- @babel/plugin-transform-object-assign 7.18.6    
- @babel/plugin-transform-react-jsx 7.18.10       
- @babel/plugin-transform-runtime 7.18.10
- @babel/preset-env 7.18.10
- @babel/preset-react 7.18.6
- @babel/register 7.18.9
- @babel/runtime 7.18.9
- @chakra-ui/icons 1.1.7
- @chakra-ui/react 1.8.8
- @emotion/cache 11.10.3
- @emotion/jest 11.10.0
- @emotion/react 11.10.0
- @emotion/styled 11.10.0
- @fluentui/react 7.196.0
- @material-ui/core 4.12.4
- @material-ui/icons 4.11.3
- @mui/icons-material 5.10.3
- @mui/material 5.10.3
- @rjsf/material-ui 5.0.0-beta.2
- @rollup/plugin-replace 4.0.0
- @types/jest-expect-message 1.0.4
- @types/json-schema 7.0.11
- @types/json-schema-merge-allof 0.6.1
- @types/lodash 4.14.184
- @types/react 17.0.48
- @types/react-dom 17.0.17
- @types/react-is 17.0.3
- @types/react-test-renderer 17.0.2
- ajv 6.12.6
- antd 4.22.8
- atob 2.1.2
- babel-loader 8.2.5
- chai 3.5.0
- chakra-react-select 3.3.8
- core-js 3.25.0
- cross-env 2.0.1
- css-loader 6.7.1
- dayjs 1.11.5
- estraverse 4.3.0
- estraverse-fb 1.3.2
- express 4.18.1
- framer-motion 5.6.0
- gh-pages 3.2.3
- html 1.0.0
- html-webpack-plugin 5.5.0
- jest 25.5.4
- jest-expect-message 1.0.2
- jest-watch-typeahead 2.1.1
- jsdom 16.7.0
- json-schema-merge-allof 0.8.1
- jsonpointer 5.0.1
- jss 10.9.2
- less 4.1.3
- less-loader 11.0.0
- loader-utils 3.2.0
- lodash 4.17.21
- lodash-es 4.17.21
- mini-css-extract-plugin 2.6.1
- mocha 5.2.0
- monaco-editor 0.33.0
- monaco-editor-webpack-plugin 7.0.1
- nanoid 3.3.4
- nyc 15.1.0
- prop-types 15.8.1
- react 17.0.2
- react-app-polyfill 1.0.6
- react-bootstrap 1.6.6
- react-dom 17.0.2
- react-frame-component 4.1.3
- react-icons 4.4.0
- react-is 18.2.0
- react-monaco-editor 0.49.0
- react-portal 4.2.2
- react-select 5.4.0
- react-test-renderer 17.0.2
- react-transform-catch-errors 1.0.2
- react-transform-hmr 1.0.4
- rimraf 3.0.2
- sass 1.54.6
- sass-loader 13.0.2
- semantic-ui-css 2.4.1
- semantic-ui-react 2.1.3
- sinon 9.2.4
- source-map-loader 4.0.0
- style-loader 3.3.1
- tsdx 0.14.1
- webpack 5.74.0
- webpack-cli 4.10.0
- webpack-dev-middleware 5.3.3
- webpack-dev-server 4.10.1
- webpack-hot-middleware 2.25.2

 ERR_PNPM_PEER_DEP_ISSUES  Unmet peer dependencies

.
├─┬ @babel/eslint-parser
│ └── ✕ missing peer @babel/core@>=7.11.0
├─┬ ts-jest
│ ├── ✕ missing peer @babel/core@">=7.0.0-beta.0 <8"
│ └── ✕ missing peer jest@^27.0.0
└─┬ dts-cli
  ├─┬ eslint-plugin-flowtype
  │ ├── ✕ missing peer @babel/plugin-syntax-flow@^7.14.5
  │ └── ✕ missing peer @babel/plugin-transform-react-jsx@^7.14.9
  ├─┬ rollup-plugin-sourcemaps
  │ └── ✕ missing peer @types/node@>=10.0.0
  └─┬ ts-node
    └── ✕ missing peer @types/node@"*"
Peer dependencies that should be installed:
  @babel/core@">=7.11.0 <8.0.0"              @babel/plugin-transform-react-jsx@^7.14.9  jest@^27.0.0
  @babel/plugin-syntax-flow@^7.14.5          @types/node@>=10.0.0

hint: If you want peer dependencies to be automatically installed, add "auto-install-peers=true" to an .npmrc file at the root of your project.
hint: If you don't want pnpm to fail on peer dependency issues, add "strict-peer-dependencies=false" to an .npmrc file at the root of your project.

@heath-freenome
Copy link
Member

Have you tried using the npm version installed with node 16? That is working for me

@ChocolateLoverRaj
Copy link
Contributor Author

I updated npm to latest version: 8.18.0, and ran npm i (which took a loooong time) and there were no errors. This is the result of running test on antd dir:

 FAIL  test/Object.test.js
  ● Test suite failed to run

    Cannot find module '@rjsf/validator-ajv6' from 'test/Object.test.js'

      1 | import React from "react";
      2 | import renderer from "react-test-renderer";
    > 3 | import validator from "@rjsf/validator-ajv6";
        | ^
      4 |
      5 | import "../__mocks__/matchMedia.mock";
      6 | import Form from "../src";

      at Resolver.resolveModule (node_modules/dts-cli/node_modules/jest-resolve/build/resolver.js:324:11)
      at Object.<anonymous> (test/Object.test.js:3:1)

@heath-freenome
Copy link
Member

Does this still happen after you do npm run build?

@ChocolateLoverRaj
Copy link
Contributor Author

ChocolateLoverRaj commented Aug 30, 2022

Does this still happen after you do npm run build?

After running npm run build everything works. Thanks.

@ChocolateLoverRaj ChocolateLoverRaj marked this pull request as ready for review August 30, 2022 18:35
Comment on lines 78 to 80
});

describe("errors", () => {
Copy link
Member

Choose a reason for hiding this comment

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

If you delete these three lines, then they will all be in a single describe block, which is what I was hoping for

@heath-freenome
Copy link
Member

@ChocolateLoverRaj All set, if you'd prefer I merge, then I can

@ChocolateLoverRaj
Copy link
Contributor Author

Yes please merge

@heath-freenome heath-freenome merged commit 94bfec8 into rjsf-team:master Aug 31, 2022
@heath-freenome
Copy link
Member

Yes please merge

Done, you'll have to delete the branch off of your fork yourself

@ChocolateLoverRaj ChocolateLoverRaj deleted the antd-empty-errors-array branch August 31, 2022 22:58
heath-freenome added a commit to heath-freenome/react-jsonschema-form that referenced this pull request Sep 3, 2022
heath-freenome added a commit to heath-freenome/react-jsonschema-form that referenced this pull request Sep 3, 2022
heath-freenome added a commit that referenced this pull request Sep 3, 2022
* Get semantic-ui theme on react 17 and dts-cli
- Updated the `package*.json` to update to react 17 and switch from `tsdx` to `dts-cli`
- Updated the tests to mock the `Ref` component from `@fluentui/react-component-ref` because it causes tests to fail

* - Ran `cs-format` over `semantic-ui` after upgrading to `dts-cli` that uses a newer version of `prettier`

* - Added the same additional Array tests that were added in #2576

* - Updated migration guide and `CHANGELOG.md`
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.

@rjsf/antd Text input shows error icon when extra errors is []
2 participants