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 React 17.x #993

Closed
mateusz-meller opened this issue Feb 13, 2020 · 26 comments
Closed

Support for React 17.x #993

mateusz-meller opened this issue Feb 13, 2020 · 26 comments

Comments

@mateusz-meller
Copy link

I'm getting the following warnings in my project:

Warning: componentWillMount has been renamed, and is not recommended for use. See https://fb.me/react-unsafe-component-lifecycles for details.

  • Move code with side effects to componentDidMount, and set initial state in the constructor.
  • Rename componentWillMount to UNSAFE_componentWillMount to suppress this warning in non-strict mode. In React 17.x, only the UNSAFE_ name will work. To rename all deprecated lifecycles to their new names, you can run npx react-codemod rename-unsafe-lifecycles in your project source folder.

Please update the following components: Uncontrolled(DateTimePicker)

Warning: componentWillReceiveProps has been renamed, and is not recommended for use. See https://fb.me/react-unsafe-component-lifecycles for details.

  • Move data fetching code or side effects to componentDidUpdate.
  • If you're updating state whenever props change, refactor your code to use memoization techniques or move it to static getDerivedStateFromProps. Learn more at: https://fb.me/react-derived-state
  • Rename componentWillReceiveProps to UNSAFE_componentWillReceiveProps to suppress this warning in non-strict mode. In React 17.x, only the UNSAFE_ name will work. To rename all deprecated lifecycles to their new names, you can run npx react-codemod rename-unsafe-lifecycles in your project source folder.

Please update the following components: Uncontrolled(DateTimePicker)

Is there any plan to support React 17.X in the near future?

@jquense
Copy link
Owner

jquense commented Feb 13, 2020

yes v5 will fully support it, I'd also take a PR to add final bit of support to v4

@olmobrutall
Copy link
Contributor

How stable is v5 already? It's already time to update the TS definitions?

@Stundji
Copy link

Stundji commented Feb 26, 2020

What about 'react-widgets-moment'? It's not compatible with react widgets v5.

@jquense
Copy link
Owner

jquense commented Feb 26, 2020

v5 is almost ready to go, there are a few rough edges that need to be filed down and some more stuff to tweak. The issue is more that i'm still trashing a bit on the list and listbox implementations around focus management. In any case we use v5 in prod so it does work. The localizers need to be released as well

@jquense
Copy link
Owner

jquense commented Feb 26, 2020

O and v5 is in typescript so no new defs needed

@olmobrutall
Copy link
Contributor

I've upgraded "react-widgets" from "4.4.11" (with type definitions) to "5.0.0-beta.9". I had some issues:

  • Many .d.ts files have imports like:
import React from 'react';  //TS1259	Build:Module '"D:/Signum/southwind/Framework/Signum.React/node_modules/@types/react/index"' can only be default-imported using the 'allowSyntheticDefaultImports' flag	
import PropTypes from 'prop-types'; //TS1192	Build:Module '"D:/Signum/southwind/Framework/Signum.React/node_modules/@types/prop-types/index"' has no default export

in order to make this imports work you need to turn on "allowSyntheticDefaultImports". Other libraries like react-router or react-bootstrap use the more correct:

import * as React from 'react';
  • Many index.d.ts files import many modules that have no have corresponding .d.ts files.

index.d.ts

import Calendar from './Calendar';
import Combobox from './Combobox';
import DatePicker from './DatePicker'; //Not found
import DateTimePicker from './DateTimePicker';  //Not found
import DropdownList from './DropdownList';
import Listbox from './Listbox';
import Localization, { DateFormats, DateLocalizer, FormatterOverrides, Localizer, NumberLocalizer, RequiredDateMethods } from './Localization';
import Multiselect from './Multiselect';
import NumberPicker from './NumberPicker';  //Not found
import SlideDownTransition from './SlideDownTransition';  //Not found
import SlideTransitionGroup from './SlideTransitionGroup';  //Not found
import TimeInput from './TimeInput';
export * from './types';
export { DropdownList, Combobox, Calendar, DatePicker, TimeInput, DateTimePicker, NumberPicker, Multiselect, Listbox, SlideTransitionGroup, SlideDownTransition, Localization, DateFormats, DateLocalizer, FormatterOverrides, Localizer, NumberLocalizer, RequiredDateMethods, };
//# sourceMappingURL=index.d.ts.map

The d.ts files are definitely not there:
image

Thanks for you time.

@olmobrutall
Copy link
Contributor

Ok... many files are still .js files, that's why. https://github.com/jquense/react-widgets/tree/v5/packages/react-widgets/src

This makes it hard to use the library from a TS project.

@jquense
Copy link
Owner

jquense commented Feb 28, 2020

I've not completed migrating everything yet, yes. Happy to take some PR's if you want to help with that! I don't know that import * as React is "more correct" react is commonjs, the import decision is fairly arbitrary until React actually publishes esm builds

@olmobrutall
Copy link
Contributor

If you use import React from “react”, since react has no default export, Babel invents a default export as the whole modules content.

This behavior simplified the initial experience of developers, but contributes to never have a clear understating of the already complicated ES modules system.

Typescript goes the strict way, making a clear distinction between importing the whole modules, or importing the default export.

Nowadays you can opt-in in typescript and you can opt-out in Babel. But by using this feature in the d.ts files your forcing consumes of “react-widgets” to opt-in, changing the typescript default.

I think not using this feature is more correct because then consumes can still make the choice themselves.

@olmobrutall
Copy link
Contributor

About contributing, if I have time I would try to convert the missing files to typescript and fix the imports. Could this be accepted?

@jquense
Copy link
Owner

jquense commented Feb 28, 2020

I happily accept PRs!

And i'm fine with either import style practically, tho I am going to get mildly pedantic about this CJS thing. First there isn't really any spec for interop between CJS and ESM. The best we can do is look at how Node is handling it because it's the only actual environment that has to support CJS and esm. And at the moment, when importing from a commonJs module there are no named exports. E.g. importing CJS from ESM only allows importing as a default.

None of that is important since this isn't about node interop, but still it's a interesting topic

@olmobrutall
Copy link
Contributor

Not sure I understood the CJS / ESM argument in Node.js. You have any link?

Anyway, I've started the typification... its a big undertaking and not easily divisible. I have removed the noImplicitAny: false options and I have 277 errors right now. I'm worried that after all the work the PR won't be accepted because of styling, conventions, etc..

I would like to clarify what things are making friction before is too late.

  • The code patterns are highly dynamic, with lot of use of high order functions, rest operators, etc.. this patterns are sometimes hard to typify and sometimes not worth at the end. For example this
  • There are dependencies that do not have type definitions. Like prop-types-extra.
  • I tent to write semicolons, I can try to avoid it but it's embedded in my muscle memory.

I would like to hear your opinion on this issues to know what is important to you.

I have made a very initial commit (maybe like the first 10%...) here: https://github.com/olmobrutall/react-widgets/commits/v5

If you want an example of the type of code that I tend to write, here are some examples:
https://github.com/signumsoftware/framework/tree/master/Signum.React/Scripts

I will also may need a review/help clarifying types that are not clear from the usage.

@jquense
Copy link
Owner

jquense commented Mar 2, 2020

Awesome thanks for starting on this. I would probably not start with trying to get noImplictAny to pass as a measure for success. At least not initially, since it's going to spiral out of control quickly. If I might suggest an approach it would be to start with leaf files and let's convert them one at a time merge and move to the next? After everything is in TS then it'd be a good time to turn on the stricter settings and work through the new errors.

It may be too much to tackle the main widget files, they do likely involve some API reworking and it may be easiest if I handle those since I've the most familiarity with the code.

And yeah some things aren't super easy to type as is. I don't have super clear advice there, I'd like to be pragmatic about it. Somethings would benefit from type safety, some less so. Like the custom prop types, probably best to do an explicit any and not worry to much about it. In general, it's good to have user faces apis have types, less important (initially) for internal ones since an any there isn't any worse than the current code. If you feel like it's taking to much time or it's a pain I'd say don't worry to much.

As far as code style goes, do whatever makes you happy, I run everything through Prettier ultimately so I'm not that picky

@olmobrutall
Copy link
Contributor

Pull request made: #996

Hope it makes the TS story more complete.

@rmckeel
Copy link

rmckeel commented Apr 8, 2020

@mateusz-meller Worth mentioning that I am seeing these same warnings on React 16.12.0, released Nov 2019. Would love to see a v4 improvement on that for those who can't upgrade to v5 right away.

@markerikson
Copy link

I see that #996 has been merged. Where does v5 stand atm? Should I be trying to use that now?

@jquense
Copy link
Owner

jquense commented Apr 16, 2020

We are using it in production so v5 is fairly stable, but still a bit rough. The docs are the major remaining factor, baring any breaking change to fix something that doesn't pan out. At the moment tho it's looking good

@markerikson
Copy link

Having not followed any of the v5 work, is there a summary of the changes over v4 anywhere (beyond fixing lifecycle warnings and doing some apparent TS conversion) ?

@jquense
Copy link
Owner

jquense commented Apr 16, 2020

unfortunately no, nothing beyond the commit history at the moment.

Major changes:

  • TS types
  • Removal of SelectList, replaced with Listbox (checkbox groups are easy enough to roll oneself in React, Listboxes not so much)
  • (Tentative) addition of TimeInput, in place of the date picker time list
  • Intl localizer support by default!
  • valueField -> dataKey
  • New (simpler) List API, only relevant for custom List components

@ssomlk
Copy link

ssomlk commented Aug 24, 2020

Facing the same issue. Using version 4.5.0

@ssomlk
Copy link

ssomlk commented Sep 5, 2020

@jquense what's the current status of this issue??

@thepuzzlemaster
Copy link

It looks like there's only been 1 commit to the v5 branch since Sept. Is there a status update on this? An estimate on remaining work?

@jquense
Copy link
Owner

jquense commented Dec 16, 2020

that status is that i am quite busy with life and work, and don't have a ton of time to devote to this. v5 is fairly stable we use it in production. I believe that it's mostly just docs left, and the current doc setup is annoyingly bespoke, and prone to breakage which makes it tough to work on :P

@thepuzzlemaster
Copy link

Totally understandable. Thank you for the update.

@olmobrutall
Copy link
Contributor

@jquense any chance to publish my PR? It’s 18 days already... I don’t need docs, test or anything, just a version that compiles on typescript.

Thanks

@jquense
Copy link
Owner

jquense commented Dec 17, 2020

yes sorry! forgot

olmobrutall referenced this issue in signumsoftware/framework Dec 20, 2020
# Conflicts:
#	Signum.React/Scripts/Signum.Entities.ts
@jquense jquense closed this as completed Mar 31, 2021
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

No branches or pull requests

8 participants