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

add PropTypes as a separate package #954

Merged
merged 8 commits into from
Oct 24, 2017
Merged

add PropTypes as a separate package #954

merged 8 commits into from
Oct 24, 2017

Conversation

suxxes
Copy link
Contributor

@suxxes suxxes commented Sep 27, 2017

Fixes #744

Description

Because @GlebDolzhikov did not respond since May, I decided to fork, fix, and pull request his original changes for PropType validations.

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x")

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

With recent React 16 release application does not work with it, because original PropTypes were removed from the core library into a separate.

What is the new behavior?

Application uses separate external prop-types library to validate incoming props.

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[x] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 61fef84 on suxxes:master into ** on adazzle:master**.

@jpdriver
Copy link
Contributor

hey @suxxes -- thank you very much for taking this on. I've been doing some internal testing and we can't merge this in just yet, as it introduces a couple of script errors we don't get on master

screen shot 2017-09-28 at 12 07 15 pm

The new prop-types package doesn't support validation the way we are currently doing it for certain props on certain components (e.g. PropTypes.arrayOf())

The exact components that throw are:-

Component Prop
AutoCompleteEditor options
ReactDataGrid metricsComputator
Grid metricsComputator
Viewport metricsComputator

metricsComputator will be a difficult one to solve. It's a prop that we're using as a context type for a mixin 😰

Mixins have been considered harmful by the React community for a while now, so potentially this move to prop-types will require a wider discussion around migrating away from using mixins as a pre-requisite.

But TL:DR the unfortunate news is I don't think we should merge this until we've gotten rid of those script errors 😞

cc @malonecj

@suxxes
Copy link
Contributor Author

suxxes commented Sep 28, 2017

Hey @jpdriver thanks for hopping in. I will look into those problems too, because I'd love to transition to R16 as soon as possible, and sadly React Data Grid is the only component left that makes me trouble with the transition.

@jpdriver
Copy link
Contributor

If you wouldn't mind updating the AutoCompleteEditor that would at least be a start 👍

The mixins stuff has been there since day one (and React 11 😂) so it's probably about time we addressed it

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 26c5039 on suxxes:master into ** on adazzle:master**.

@giulianf
Copy link

giulianf commented Sep 30, 2017

Hi @jpdriver
When do you schedule to merge ?

@suxxes
Copy link
Contributor Author

suxxes commented Sep 30, 2017

@jpdriver I've fixed all the issues I could find (including docs and examples), all that I see now are that of metricsComputator.

@giulianf
Copy link

giulianf commented Oct 5, 2017

Nothing about this PR?

@IanMarett
Copy link

This PR is the only thing holding back migrating to 16. Any timescales on the merge?

@suxxes
Copy link
Contributor Author

suxxes commented Oct 10, 2017

@IanMarett I am sorry, but the whole problem with merging this package and finalizing transition to React 16 is that in latter React.createClass functionality has been deprecated and removed completely. Which forces a decent rewrite of certain functionality of the component to make it work properly, because it uses React.createClass alongside with Mixins.

@philquinn
Copy link
Contributor

@suxxes I have a PR with createClass being replaced. #970

@suxxes
Copy link
Contributor Author

suxxes commented Oct 16, 2017

@philquinn Awesome! Once it's merged I will merge master into my repo and fix merge issues if any for this PR to go live too. Thank you!

@malonecj
Copy link
Contributor

Hi @suxxes, #970 has been merged in so feel free to catch up and resolve conflicts. We will then look to get this merged as soon as possible

@suxxes
Copy link
Contributor Author

suxxes commented Oct 18, 2017

Hi @malonecj. Right on to it!

Conflicts:
	packages/react-data-grid-addons/src/editors/AutoCompleteEditor.js
	packages/react-data-grid-addons/src/editors/DateRangeEditor.js
	packages/react-data-grid-addons/src/editors/widgets/DateRangeFilter.js
	packages/react-data-grid-addons/src/formatters/DropDownFormatter.js
	packages/react-data-grid-addons/src/formatters/ImageFormatter.js
	packages/react-data-grid-addons/src/toolbars/Toolbar.js
	packages/react-data-grid-examples/src/scripts/example05-custom-formatters.js
	packages/react-data-grid-examples/src/scripts/example12-customRowRenderer.js
	packages/react-data-grid-examples/src/scripts/example14-all-features-immutable.js
	packages/react-data-grid-examples/src/scripts/example18-context-menu.js
	packages/react-data-grid-examples/src/scripts/example21-grouping.js
	packages/react-data-grid-examples/src/scripts/example23-immutable-data-grouping.js
	packages/react-data-grid-examples/src/scripts/example23-row-reordering.js
	packages/react-data-grid/src/Cell.js
	packages/react-data-grid/src/Grid.js
	packages/react-data-grid/src/ReactDataGrid.js
	packages/react-data-grid/src/Row.js
	packages/react-data-grid/src/cells/headerCells/FilterableHeaderCell.js
	packages/react-data-grid/src/cells/headerCells/SortableHeaderCell.js
	packages/react-data-grid/src/editors/CheckboxEditor.js
	packages/react-data-grid/src/editors/EditorContainer.js
	packages/react-data-grid/src/formatters/SimpleCellFormatter.js
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 57f7bfb on suxxes:master into ** on adazzle:master**.

@dijonkitchen
Copy link

FYI, this fixes #744, so you can add that to your description and auto-close it!

@suxxes
Copy link
Contributor Author

suxxes commented Oct 18, 2017

Thank you @dijonkitchen. Added issue info to description.

@suxxes
Copy link
Contributor Author

suxxes commented Oct 19, 2017

@malonecj Just in case: I've merged master and PR is ready.

@ghost
Copy link

ghost commented Oct 20, 2017

Hi, do you have a rough idea of when this might make it into a release? I'm currently working on a React 16 project and I like the look of this grid. Many thanks!

@philipbjorge
Copy link

I'm seeing a bunch of the following warnings in the console when running this fork locally.

Warning: getDefaultProps is only used on classic React.createClass definitions. Use a static property named defaultProps instead.

(Brand new to react, so no idea what this means)

@malonecj
Copy link
Contributor

Looks like we are still getting some prop type warnings on example pages. Looks to be from 3rd party libraries - an old version of React Router. I will merge this in and we can address those in a new PR. Cheers!!

@Waize
Copy link

Waize commented Nov 28, 2017

still getting warnings in addons in Portal-Component:
propTypes: { container: _react2.default.PropTypes.oneOfType([_componentOrElement2.default, _react2.default.PropTypes.func]) },

sorry I can't provide you the right sourcefile.

@suxxes
Copy link
Contributor Author

suxxes commented Nov 28, 2017

@Waize I looked through all the files which contain usage of PropTypes.oneOfType, all of them have new PropTypes imported. You either forgot to update, or it's something newly added after the migration to external PropTypes.

@Waize
Copy link

Waize commented Nov 29, 2017

I am using react-data-grid-addons in version 2.0.76

@john-hadron
Copy link

I get the same error. I think the generated react-data-grid-addons.js was not updated?

file:package.json

    "react-data-grid": "^2.0.78",
    "react-data-grid-addons": "^2.0.78",

file: node_modules/react-data-grid-addons/dist/react-data-grid-addons.js

var Modal = _react2.default.createClass({
	  displayName: 'Modal',


	  propTypes: _extends({}, _Portal2.default.propTypes, {

	    /**
	     * Set the visibility of the Modal
	     */
	    show: _react2.default.PropTypes.bool,

	    /**
	     * A Node, Component instance, or function that returns either. The Modal is appended to it's container element.
	     *
	     * For the sake of assistive technologies, the container should usually be the document body, so that the rest of the
	     * page content can be placed behind a virtual backdrop as well as a visual one.
	     */
	    container: _react2.default.PropTypes.oneOfType([_componentOrElement2.default, _react2.default.PropTypes.func]),
...

file: node_modules/react-data-grid-addons/package.json

{
  "name": "react-data-grid-addons",
  "version": "2.0.78",
  "description": "A set of addons for react-data-grid",
  "scripts": {
    "beforepublish": "node ../../ci/publish/replacePackageEntry react-data-grid-addons true",
    "postpublish": "node ../../ci/publish/replacePackageEntry react-data-grid-addons",
    "test": "npm test"
  },
  "keywords": [
    "react",
    "react-data-grid",
    "react-data-grid-addons"
  ],
  "author": "Adazzle",
  "license": "MIT",
  "dependencies": {
    "react-data-grid": "^2.0.78"
  },
  "devDependencies": {
    "jquery": "^2.1.1",
    "lodash": "^4.13.1",
    "lodash.groupby": "^4.5.1"
  }
}

@29er
Copy link

29er commented Dec 14, 2017

same as @john-hadron. what's the status of this ? I am getting the same errors and starting to realize I can't use this grid anymore :(

@rkram5424
Copy link

I too am having an issue with 'oneOfType' of undefined originating from react-data-grid-addons. Is anyone working on this? It would be nice to use it with react 16.

@charleyw
Copy link

charleyw commented Feb 2, 2018

I too am having an issue with 'oneOfType' of undefined originating from react-data-grid-addons. Is anyone working on this? It would be nice to use it with react 16.

I have the same issue, and after a short investigation. I found it is caused by one of the dependency of contextMenu in react-data-grid-addon.

https://github.com/adazzle/react-data-grid/blob/master/package.json#L36

"react-contextmenu": "1.6.2",

[email protected] depends on react-overlays@^0.6.3, and react-overlays@^0.6.3 is using old proptype.

@utajum
Copy link

utajum commented Feb 2, 2018

There is an ongoing pull request #1081 (comment)

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.

Getting warnings with react 15.5+