Conversation
demiankatz
left a comment
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
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
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.
| ] | ||
| <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.
formating Co-authored-by: Demian Katz <demian.katz@villanova.edu>
There was a problem hiding this comment.
See below for a couple more comments based on another partial review (I'm still just reviewing random files in an effort to chip away at the list -- have about 100 more to look at).
I also ran into a problem in hands-on testing; when I try to create a new object, it doesn't work, and I get this error:
...seems to be the same issue I reported above, so that likely needs another look.
I also reviewed the date picker problem I noted above. While there is no longer a fatal error, there is a new problem: you used to be able to pick date AND time; now you can only pick date.
| const tree = renderer.create(<DatastreamUploadModalContent />).toJSON(); | ||
| expect(tree).toEqual("DatastreamLicenseContent"); | ||
| const { container } = render(<DatastreamUploadModalContent />); | ||
| expect(container.innerHTML).toContain("DatastreamLicenseContent"); |
There was a problem hiding this comment.
Is there a reason that we changed toEqual to toContain here? Using toContain is a bit less precise; maybe it's justified if innerHTML contains messy content, but if that's the case, perhaps the messy content could be normalized with trim() or a regular expression to enable restoration of toEqual. I can live with toContain if we really have to, but I'd prefer to retain specificity if we can.
| expect(mockDatatypeContent).toHaveBeenCalledWith(response); | ||
| const { asFragment } = render(<DatastreamViewModalContent />); | ||
| await waitFor(() => expect(datastreamOperationValues.viewDatastream).toHaveBeenCalled()); | ||
| expect(asFragment()).toMatchSnapshot(); |
There was a problem hiding this comment.
The snapshots for this test don't look right to me -- I think they're capturing the progress bar but not the actual result. Is it possible the snapshot assessment needs to be moved below the waitFor here? (That's just a guess -- maybe there's a different solution... but something doesn't seem right in the snapshot file).
2 participants
Working on client side dependencies