Conversation
demiankatz
left a comment
There was a problem hiding this comment.
The upgraded tree view isn't working:
The tree view is used to render the object type selection control when you create a new object (e.g. at http://localhost:3000/edit/newChild).
Sounds like there's a new unique ID requirement on the <TreeItem> element -- maybe this is as easy as renaming or copying the existing nodeId element to id in the code.
The upgraded date picker also isn't working:
...you can test this by editing the PROCESS-MD datastream on any object. The cause of this error is less clear-cut from the message, but hopefully TypeError: value.isValid is not a function will point toward a particular bit of code that can be revised or updated. If you get stuck, let me know and I'll refresh my memory... or maybe looking at the component changelogs will clarify what's happening.
demiankatz
left a comment
There was a problem hiding this comment.
The lint failure in #359 led me back here to see what the lint status was in this PR... and I see that linting is failing entirely because next lint --fix seems to be removed in this version of Next.js.
Suggested order of operations:
1.) Maybe we should pivot to fixing types in React component definitions so that lint will pass in #359. I realize this is probably a bit of a large project, but it's also something we need to do anyway, and it may make the upgrading and testing easier in the long run. I think it's worth considering as an investment. Maybe we can start by picking one or two random components and improving their typing, to see how it works and how much effort it takes. If that goes well, we can chip away at the rest in a series of PRs. (I suggest doing only a few files per PR, so if we get into review cycles, we can work through multiple things in parallel rather than having a million conversations in a single monster PR).
2.) Maybe we should also look into replacing next lint with eslint configuration ahead of upgrading Next.js, so we'll have the benefit of linting while we work through the upgrade. I suspect if we fix the linting issues in this PR, we can then "backport" those changes to a separate PR that can get merged ahead of the rest of the upgrades.
What do you think?
…t.ReactElement =>'
demiankatz
left a comment
There was a problem hiding this comment.
I decided to get a start at looking over this tonight -- managed to check off 57 random files out of the total of 182. I'll do more tomorrow! In any case, the majority of what I've seen so far looks great. See below for some questions and suggestions.
| rules: { | ||
| "@typescript-eslint/no-require-imports": "off" | ||
| }, |
There was a problem hiding this comment.
Looks like there was a mix of tabs and spaces here; converting to spaces so things line up consistently:
| rules: { | |
| "@typescript-eslint/no-require-imports": "off" | |
| }, | |
| rules: { | |
| "@typescript-eslint/no-require-imports": "off" | |
| }, |
| import JobPaginator from "../../../components/paginate/JobPaginator"; | ||
|
|
||
| export default function CategoryJob(): React.ReactElement { | ||
| export default function CategoryJob(): React.ReactElement<any> { |
There was a problem hiding this comment.
I see there are still some of these React.ReactElement<any> types in this PR. Do we really need these? I thought we tried to separate them out to a different PR and ultimately decided to get rid of them.
| ] | ||
| <DocumentFragment> | ||
| {"pid":"foo:123"}, | ||
| </DocumentFragment> |
There was a problem hiding this comment.
I think there's a problem with this test -- it was originally rendering a <div> and now we're only getting the JSON part. I think for the test to be meaningful, we need to verify the message rendering.
| />, | ||
| ] | ||
| `; | ||
| exports[`JobSelector renders 1`] = `<DocumentFragment />`; |
There was a problem hiding this comment.
Something seems wrong here -- the snapshot is empty now.
Working on client side dependencies