Upgrade client dependencies (including Next 16/React 19).#358
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 =>'
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.
| 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).
There was a problem hiding this comment.
This also still appears to be an issue.
There was a problem hiding this comment.
I reordered it and it did change the snapshot to include additional data.
There was a problem hiding this comment.
This looks better, but the "DatastreamViewModalContent renders for download-only content" snapshot is still showing a progress icon instead of the expected result. You did fix "DatastreamViewModalContent renders for viewable content" though, so we're at least halfway there!
There was a problem hiding this comment.
I reordered it and I think I got better results.
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks for the progress here, @Jason-Benson! I still haven't reviewed every single file, but see below for a few more questions and suggestions.
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @Jason-Benson, I've done a bunch more hands-on testing and wasn't able to break anything. I also looked through all of the remaining files and spotted a handful of remaining issues (some of which may be safe to ignore, but I always err on the side of caution).
Please let me know if you want to discuss any of this stuff further -- the end is really in sight now!!
| await act(async () => { | ||
| asFragment = render(getChildComponent(props)).asFragment; | ||
| }); | ||
| expect(asFragment()).toMatchSnapshot(); |
There was a problem hiding this comment.
Would it be safer to check the snapshot AFTER we've asserted that the data has been fetched, or does it not matter in this case? Just wondering about the reason for the changed order of operations here (and a few other places in this test file).
There was a problem hiding this comment.
Today I changed the order to see if it made a difference. It didn't, but I'll leave it in that order since it more closely matches the original code before I touched it.
There was a problem hiding this comment.
What I'm seeing in this PR still has the order of operations changed, but your comment suggests that you realigned the code to more closely match the original order. Did something not get pushed, or is it possible that you changed one test case in the file but not all of them?
There was a problem hiding this comment.
I see what happened here. I reordered the rest of the tests in Child.test.tsx, but somehow forgot the very one you had flagged. Fixed.
There was a problem hiding this comment.
Looking better, but I think there's just one more that needs tweaking: "renders child counts" on line 81.
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks for the progress, @Jason-Benson! I've resolved some completed threads, commented on others that still have pending actions, and spotted a couple of problems highlighted below (which I think should be easy to fix -- looks like a merge conflict got committed without being fully resolved).
| tree = getMountedObjectStatusComponent(props); | ||
| await waitFor(() => expect(global.fetch).toHaveBeenCalledTimes(1)); | ||
| }); | ||
| const { asFragment } = getMountedObjectStatusComponent(props); |
There was a problem hiding this comment.
Do we need an await act here? The snapshot for this seems to have gone from showing an unknown status to showing the loading indicator, which makes me suspect things aren't fully loading/rendering before the snapshot is generated.
There was a problem hiding this comment.
Yeah, that did it.
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Victory at last -- thanks, @Jason-Benson, this all looks good to me now!
demiankatz
changed the title
2 participants
Working on client side dependencies