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

Support for defaultProps will be removed from function components in a future major release. Use JavaScript default parameters instead. at {Component} #3615

Closed
marco-digennaro opened this issue Jun 7, 2023 · 118 comments
Assignees
Labels
P0 Critical priority issues refactor PRs that refactor existing code

Comments

@marco-digennaro
Copy link

Using a library that include recharts v2.6.2 i get the following error in the console.

Curve: Support for defaultProps will be removed from function components in a future major release. Use JavaScript default parameters instead. at Curve

the charts are rendered normally thou.

@ckifer ckifer added the refactor PRs that refactor existing code label Jun 7, 2023
@ckifer
Copy link
Member

ckifer commented Jun 7, 2023

Thanks, noted! Unfortunately moving away from defaultProps causes unintended bugs due to the way recharts works. But this is known and we're trying to get away from them where we can.

Edit: see solution in 2.x #3615 (comment)

@cdimitroulas
Copy link

FYI this also happens for the Tooltip component

@ckifer
Copy link
Member

ckifer commented Jun 9, 2023

Reference for why this is an issue #3438 (comment)

@ckifer ckifer added the P1 High priority issues label Jun 12, 2023
@ckifer
Copy link
Member

ckifer commented Jun 15, 2023

@akamfoad all of our functional refactors are going to have this issue. We may have to prepared to handle soon as I'm going to release 2.7. Let me know if you have any thoughts

@akamfoad
Copy link
Contributor

@ckifer
I was always feeling bad about the defaultProps but in my refactors I kept them just in case and to reduce the number of changes.

But I think the sooner we embrace JS default params the better, I see many teams are going in this direction, especially those who use TypeScript, I see that is quite the default practice.

This is not going to break semver if we do it in v2, because we just change how we receive the default values, internally, for the users of the library everything will be the same and the warning goes away. What do you think?

@ckifer
Copy link
Member

ckifer commented Jun 15, 2023

@akamfoad I totally agree, but we still have the cloneElement issue linked above (#3438 (comment)).

I believe all of the components we have refactored besides Tooltip are safe from that issue so we should be able to move them over

@ckifer
Copy link
Member

ckifer commented Jun 15, 2023

I believe this list holds those that we cannot touch without fear of breaking things

@akamfoad
Copy link
Contributor

So only those components will cause issue if defaultProps is removed, that another component/function needs to access them, including cloneElement, before the component is initialized.

I'm guessing there is a lot of them, especially those from the list in generateCategoricalChart.

Which means they in fact cause breaking change, when migrated to default params. So we might need to hold off here and revert the Tooltip refactor, that is an easy one.

The warning is shown only for Functional Components with defaultProps, but the default behavior may changes in the future for how this affects components (if I understood the RFC correctly). Which means, its probably for the best to prepare for v3.

@akamfoad
Copy link
Contributor

Btw @ckifer

I think its probably for the best, this might encourage us to have major version (not very soon tho), where we internally do not rely on defaultProps, or cloneElement, instead we use normal JS default params, and renderProps (which is not my personal favorite), or Context API.

This way, the children can both render and subscribe to context's provided by the charts (or a an HoC) parent and render, clutter-free

@ckifer
Copy link
Member

ckifer commented Jun 15, 2023

Thanks for the thoughts @akamfoad! Yeah I agree - I've been shying away from 3.x because it's a lot of work and there's a lot of breaking changes we need to merge in and document but there hasn't really been any meaningful progress made towards it.

But yes we definitely need to 100% rid ourselves of the cloneElement approach, it's the cause of a lot of issues (but it's also some of the magic of recharts which is why it's tough to remove or at least tough to test once you remove it).

@jocarrd
Copy link

jocarrd commented Jun 20, 2023

image
Same problem, any solution?

@ckifer
Copy link
Member

ckifer commented Jun 20, 2023

No solution yet but we're aware of it, thanks

@shohan-pherones
Copy link

I also need the solution asap, do your job devs.

@ckifer
Copy link
Member

ckifer commented Jul 8, 2023

There are no devs that work on this project as a job. Please feel free to contribute.

@zwhitchcox
Copy link

Just leaving this here:

const error = console.error;
console.error = (...args: any) => {
  if (/defaultProps/.test(args[0])) return;
  error(...args);
};

@ooanishoo
Copy link

zwhitchcox

Hi, is this a temporary fix that we can implement for now?

@zwhitchcox
Copy link

@ooanishoo This will just hide the warning, but there's not really anything else you can do until it's fixed upstream anyway.

@wuahi
Copy link

wuahi commented Jul 25, 2023

Having the same issue. Is there an update on where this might be fixed?

@evanlong0803
Copy link

I was just about to ask a question, but I guess I won't. Everyone knows 🤣

@ckifer ckifer added P0 Critical priority issues and removed P1 High priority issues labels Jul 25, 2023
@ckifer
Copy link
Member

ckifer commented Jul 25, 2023

No update really besides some noodling in my head to try to pick a path forwards. If I work on anything at all next it will be this, I just can't make time for it right now as other (life) priorities prevail.

@ckifer
Copy link
Member

ckifer commented Jul 26, 2023

Risky elements are defined (by me at least 😅 ) as any that are referenced by a call to findAllByType findChildByType and those in the map in generateCategoricalChart (see issue linked above - #3438 (comment)). This comes out to:

  1. Cell
  2. Brush
  3. Tooltip
  4. Legend
  5. ErrorBar
  6. XAxis
  7. YAxis
  8. ZAxis
  9. PolarAngleAxis
  10. PolarRadialAxis
  11. RadialBar
  12. Label
  13. LabelList
  14. ReferenceDot
  15. ReferenceLine
  16. ReferenceArea
  17. CartesianGrid
  18. PolarGrid
  19. Customized
  20. Any graphical child - Bar, Area, Line, Scatter, Funnel, Pie, Radar, RadialBar

All other components outside of these should be able to be refactored to use default params with relative certainty they won't break things. This makes this issue actionable and means that Text, Curve and any others not on this list can be moved over any time. @nikolasrieble @akamfoad for vis.

If anyone wants to take the changes or the change for one component let me know.

@ckifer
Copy link
Member

ckifer commented Jul 26, 2023

Text refactored in #3670. Curve is open for contribution as well as any others that may currently have issue

@diogotrindadeversion1
Copy link

diogotrindadeversion1 commented May 23, 2024

Removing defaultProps do not make any sense by the React team.
defaultProps have been a standard on the documentation for years, always on the main examples. And it helps organizing the code with less nested objects. I believe that defaultProps is a better solution comparing to nesting things with JavaScript.
Lots of libraries will have the same issue, including custom private libraries of components, that does not make sense to refactor everything, just because yes.

So I think that you should one an issue on the React repository.
They have a new compiler, they should use it for optimization, and keep defaultProps alone/working.

@PavelVanecek
Copy link
Collaborator

defaultProps will stay on class components. This warning is about defaultProps on functional components.

So we could technically solve this by rewriting everything from functional components to classes. But that also means rewriting all hooks.

@diogotrindadeversion1
Copy link

defaultProps will stay on class components. This warning is about defaultProps on functional components.

So we could technically solve this by rewriting everything from functional components to classes. But that also means rewriting all hooks.

That does not make any sense. Using classes is going back in time, and it is an old pattern. We can have functions with defaultProps.
Everybody should have that conversation on the React repository.

@ckifer
Copy link
Member

ckifer commented May 24, 2024

Alpha release - https://www.npmjs.com/package/recharts/v/2.13.0-alpha.0
This BREAKS things in R19 (though they're already broken), shouldn't in R18

Please use this alpha to see if defaultProps warnings are gone (and make sure things are still working as normal in R18 and below). For now, all the function components with issues have been wrapped with classes (while we work on 3.x)

@mmdev73
Copy link

mmdev73 commented May 24, 2024

Alpha release - https://www.npmjs.com/package/recharts/v/2.13.0-alpha.0 This BREAKS things in R19 (though they're already broken), shouldn't in R18

Please use this alpha to see if defaultProps warnings are gone (and make sure things are still working as normal in R18 and below). For now, all the function components with issues have been wrapped with classes (while we work on 3.x)

Works perfectly with R18, console errors disappear and no broken things found. Great job and thanks a lot.

@ckifer
Copy link
Member

ckifer commented May 24, 2024

Nice! All credit to @eps1lon for the interim solution.

Work still needed for things not to break in R19 so I'll wait for more feedback and see if we can get full support in before resolving this. Thanks everyone.

@ckifer
Copy link
Member

ckifer commented May 24, 2024

https://www.npmjs.com/package/recharts/v/2.13.0-alpha.3
Should have more R19 support if not all of it (minus the need to disregard peerDeps and install react-is@19)

Please test this one as well on all React versions 16.8-19 to see if anyone runs into issues. Thanks! 🚀

@MD-YeakubNabi-Hridoy
Copy link

Warning: XAxis: Support for defaultProps will be removed from function components in a future major release. Use JavaScript default parameters instead.
how to fix this

@ckifer
Copy link
Member

ckifer commented May 26, 2024

Try installing the alpha version listed above

@Wallauerr
Copy link

@ckifer it worked!
The console error is gone.

@barhouum7
Copy link

barhouum7 commented May 27, 2024

Hey y'all, I think installing v2.10.2 should fix the warning, so try this:

npm install [email protected]

[UPDATE]: v2.10.2 was working perfectly in a React version under R18

For React versions above R18, this works perfectly for me npm i [email protected]

@marcusvinicius0
Copy link

Console error is gone, thx a lot for the support!

@ckifer
Copy link
Member

ckifer commented May 28, 2024

I'm going to close this since we have a path forwards to remove the errors

Solution is npm i recharts@alpha
I will post back here again when R19 is out and I can release 2.13.0, that will contain the alpha fixes.

Feel free to continue to report any issues found here (or R19 related here)

Thanks all

@ckifer ckifer closed this as completed May 28, 2024
@jewseppi
Copy link

jewseppi commented Jun 3, 2024

after installing the alpha version, i started getting an error on my build server about a missing dependency (ajv). had to add the library to package.json to resolve.

@ckifer
Copy link
Member

ckifer commented Jun 3, 2024

@jewseppi do you mind posting the error here? We don't directly depend on or use Ajv so I'm confused

@jewseppi
Copy link

jewseppi commented Jun 3, 2024

Error: Cannot find module 'ajv/dist/compile/codegen'
Require stack:

  • D:\a\1\s\retailservices.web.management\node_modules\ajv-keywords\dist\definitions\typeof.js
  • D:\a\1\s\retailservices.web.management\node_modules\ajv-keywords\dist\keywords\typeof.js
  • D:\a\1\s\retailservices.web.management\node_modules\ajv-keywords\dist\keywords\index.js
  • D:\a\1\s\retailservices.web.management\node_modules\ajv-keywords\dist\index.js
  • D:\a\1\s\retailservices.web.management\node_modules\schema-utils\dist\validate.js
  • D:\a\1\s\retailservices.web.management\node_modules\schema-utils\dist\index.js
  • D:\a\1\s\retailservices.web.management\node_modules\mini-css-extract-plugin\dist\index.js
  • D:\a\1\s\retailservices.web.management\node_modules\react-scripts\config\webpack.config.js
  • D:\a\1\s\retailservices.web.management\node_modules\react-scripts\scripts\build.js
    at Module._resolveFilename (node:internal/modules/cjs/loader:1186:15)
    at Module._load (node:internal/modules/cjs/loader:1012:27)
    at Module.require (node:internal/modules/cjs/loader:1271:19)
    at require (node:internal/modules/helpers:123:16)
    at Object. (D:\a\1\s\retailservices.web.management\node_modules\ajv-keywords\dist\definitions\typeof.js:3:19)
    at Module._compile (node:internal/modules/cjs/loader:1434:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1518:10)
    at Module.load (node:internal/modules/cjs/loader:1249:32)
    at Module._load (node:internal/modules/cjs/loader:1065:12)
    at Module.require (node:internal/modules/cjs/loader:1271:19) {
    code: 'MODULE_NOT_FOUND',
    requireStack: [
    'D:\a\1\s\retailservices.web.management\node_modules\ajv-keywords\dist\definitions\typeof.js',
    'D:\a\1\s\retailservices.web.management\node_modules\ajv-keywords\dist\keywords\typeof.js',
    'D:\a\1\s\retailservices.web.management\node_modules\ajv-keywords\dist\keywords\index.js',
    'D:\a\1\s\retailservices.web.management\node_modules\ajv-keywords\dist\index.js',
    'D:\a\1\s\retailservices.web.management\node_modules\schema-utils\dist\validate.js',
    'D:\a\1\s\retailservices.web.management\node_modules\schema-utils\dist\index.js',
    'D:\a\1\s\retailservices.web.management\node_modules\mini-css-extract-plugin\dist\index.js',
    'D:\a\1\s\retailservices.web.management\node_modules\react-scripts\config\webpack.config.js',
    'D:\a\1\s\retailservices.web.management\node_modules\react-scripts\scripts\build.js'
    ]
    }

Node.js v22.2.0
##[warning]Couldn't find a debug log in the cache or working directory
##[error]Error: Npm failed with return code: 1

@jewseppi
Copy link

jewseppi commented Jun 3, 2024

I'm also seeing this:

ERROR in ./node_modules/recharts/es6/index.js
Module build failed (from ./node_modules/source-map-loader/dist/cjs.js):
Error: ENOENT: no such file or directory, open 'C:\DevOps\RetailServices.Web\ret
ailservices.web.management\node_modules\recharts\es6\index.js'

@ckifer
Copy link
Member

ckifer commented Jun 3, 2024

Thanks

npm ls ajv in recharts results in

recharts % npm ls ajv
[email protected] /Volumes/workplace/recharts
├─┬ @storybook/[email protected]
│ └─┬ @storybook/[email protected]
│   └─┬ [email protected]
│     └─┬ [email protected]
│       ├─┬ [email protected]
│       │ └── [email protected] deduped
│       └── [email protected]
├─┬ [email protected]
│ └─┬ [email protected]
│   ├─┬ [email protected]
│   │ └── [email protected]
│   ├─┬ [email protected]
│   │ └── [email protected] deduped
│   └── [email protected]
├─┬ [email protected]
│ ├─┬ @eslint/[email protected]
│ │ └── [email protected] deduped
│ ├── [email protected]
│ └─┬ [email protected]
│   └── [email protected]
├─┬ [email protected]
│ └─┬ [email protected]
│   ├─┬ [email protected]
│   │ └── [email protected] deduped
│   └── [email protected] deduped
└─┬ [email protected]
  ├─┬ [email protected]
  │ ├─┬ [email protected]
  │ │ └── [email protected] deduped
  │ └── [email protected]
  └─┬ [email protected]
    └─┬ [email protected]
      ├─┬ [email protected]
      │ └── [email protected] deduped
      └── [email protected]

So I don't think this is on the recharts side of things, everything listed there are devDependencies

@JJozef
Copy link

JJozef commented Jun 3, 2024

The "recharts" version: "2.13.0-alpha.1", not a single problem (with nextjs 14).

@ckifer
Copy link
Member

ckifer commented Jun 3, 2024

@JJozef there are some bugs if you upgrade to React 19, but glad to hear no regressions otherwise. The latest alpha should be good in R19 (at least no issues thus far).

@jewseppi I can npm i recharts@alpha and see the node_modules\recharts\es6\index.js exists. Not sure why its being weird for you
image

@jewseppi
Copy link

jewseppi commented Jun 3, 2024

Okay thanks for the quick response.

@ElhadjBoubacar
Copy link

Android Bundled 6042ms C:\Users\MON P C\Desktop\Rappelle\mon-app\node_modules\expo\AppEntry.js (1051 modules)
ERROR Warning: Unknown: Support for defaultProps will be removed from memo components in a future major release. Use JavaScript default parameters instead.
in RCTView (created by View)
in View (created by Rendez)
in RCTView (created by View)
in View (created by Rendez)
in RCTView (created by View)
in View (created by Rendez)
in RCTView (created by View)
in View (created by Rendez)
in Rendez (created by SceneView)
in StaticContainer
in EnsureSingleNavigator (created by SceneView)
in SceneView (created by BottomTabView)
in RCTView (created by View)
in View (created by Screen)
in RCTView (created by View)
in View (created by Background)
in Background (created by Screen)
in Screen (created by BottomTabView)
in RNSScreen (created by Animated(Anonymous))
in Animated(Anonymous) (created by InnerScreen)
in Suspender (created by Freeze)
in Suspense (created by Freeze)
in Freeze (created by DelayedFreeze)
in DelayedFreeze (created by InnerScreen)
in InnerScreen (created by Screen)
in Screen (created by MaybeScreen)
in MaybeScreen (created by BottomTabView)
in RNSScreenContainer (created by ScreenContainer)
in ScreenContainer (created by MaybeScreenContainer)
in MaybeScreenContainer (created by BottomTabView)
in RNCSafeAreaProvider (created by SafeAreaProvider)
in SafeAreaProvider (created by SafeAreaProviderCompat)
in SafeAreaProviderCompat (created by BottomTabView)
in BottomTabView (created by BottomTabNavigator)
in PreventRemoveProvider (created by NavigationContent)
in NavigationContent
in Unknown (created by BottomTabNavigator)
in BottomTabNavigator (created by App)
in EnsureSingleNavigator
in BaseNavigationContainer
in ThemeProvider
in NavigationContainerInner (created by App)
in App (created by withDevTools(App))
in withDevTools(App)
in RCTView (created by View)
in View (created by AppContainer)
in RCTView (created by View)
in View (created by AppContainer)
in AppContainer
in main(RootComponent)

@ckifer
Copy link
Member

ckifer commented Jun 7, 2024

@ElhadjBoubacar please upgrade to the alpha version if you want the warning to go away. Will release 2.13 when R19 is released

@Yumat10
Copy link

Yumat10 commented Jun 10, 2024

This is awesome, thank you all for the hard work!! 🙌

@CodingReaper1
Copy link

i tried downloading alpha but it didnt work console error didnt go away Warning: XAxis: Support for defaultProps will be removed from function components in a future major release. Use JavaScript default parameters instead.

@ckifer
Copy link
Member

ckifer commented Jun 16, 2024

There's only one implementation, so there can't be some people that this is fixed for and others that it isn't. Please npm ls recharts (or equivalent) and make sure you're using the alpha version

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 Critical priority issues refactor PRs that refactor existing code
Projects
None yet
Development

No branches or pull requests