Frontend dependency updates #5721
Conversation
github-actions
bot
added
scope: frontend
scope: infrastructure
dependencies
tsmock
force-pushed
the
chore/frontend-dependency-cleanup
branch
2 times, most recently
from
3398ff9
to
7450a7a
Compare
There was a problem hiding this comment.
I have reviewed most of the changes overall and find them satisfactory. I have provided my comments where I felt there was room for improvement or further attention. Great job on addressing the annoying warnings across the app! I can't help but add a little levity here:
"Knock, knock."
"Who's there?"
"Warnings."
"Warnings who?"
"No warnings because Taylor has squashed them all!"
It's fantastic to see most of those warnings disappear. There are currently some failing test cases in the codebase that requires revision. I'd like to reassess the modifications when it's rebased to develop. Thanks.
EDIT:
The image in the testimonial section on the landing page is not displayed.
| scales: { xAxes: [{ type: 'time', time: { unit: unit } }] }, | ||
| scales: { | ||
| y: { ticks: { beginAtZero: true } }, | ||
| x: { type: 'time', time: { unit: unit }, adapters: { date: { locale: enUS } } }, |
There was a problem hiding this comment.
There seems to be a noticeable difference in the appearance of the chart and units before and after the change.
Before:
After:
Furthermore, it appears that there is a new addition to the chart tooltip, displaying the time. Previously, the time was not included in the tooltip.
There was a problem hiding this comment.
I didn't consider this -- I was testing against a local DB, so I had very few data points. The problem here is that the latter is more accurate when showing when contributions occurred relative to other data points. I don't know who implemented the original timeline, but we might want to loop them in for this conversation. I can see it going either way. There is a timeseries type we can use if the original plot was the intended behavior.
There was a problem hiding this comment.
I don't have knowledge of the original timeline implementation. @ramyaragupathy, I'm looping you into this conversation. You might have more insights or context that can help us make an informed decision.
There was a problem hiding this comment.
@HelNershingThapa @tsmock - monthly view would work only for projects that are running for a longer time, for ones that have been active for only a week or so would the x axis automatically be using date instead of month? Ideally it will be good to have a toggle between month and date view for the chart. For now we can stick to dates instead of months for the x-axis.
There was a problem hiding this comment.
I've pulled the fixes for the console warnings out into #5964.
I haven't hit all the warnings. Some would require significant changes (for example, there is a warning on the project list about
Yep. I had them all passing before my last rebase, and was going to work through them this week.
Thanks for noticing. I'll double check and see what happened. I don't recall removing any images/image code. |
I think I ran into the same problem when upgrading to React 18. I was able to resolve it with b7ff508. |
tsmock
force-pushed
the
chore/frontend-dependency-cleanup
branch
from
7edbab9
to
3af2c03
Compare
|
The test execution using fetch generates numerous warnings that indicate a captured request without a corresponding request handler in the MSW library. Here's the specific warning message: No warning was encountered when testing a component that utilizes Axios for data fetching. It seems likely that the warnings are associated with MSW's specific handling mechanisms for the native fetch API in Node.js 18. EDIT: MSW will ship with Node.js 18+ support with MSW v2.0 as mentioned here mswjs/msw#1388 (comment). |
tsmock
force-pushed
the
chore/frontend-dependency-cleanup
branch
from
7f91467
to
5377f20
Compare
tsmock
force-pushed
the
chore/frontend-dependency-cleanup
branch
2 times, most recently
from
91ecdad
to
dbec7f5
Compare
|
For the MSW issue, I did a bit of debugging. It turns out this is a problem from the This is fixed in later versions of The remaining tests that are failing are due to timeout issues, and are not very consistent. Should I increase the timeouts for them? |
|
I find it puzzling that this seemingly simple test takes longer than the 5-second timeout. Because the test appears to be a straightforward simple one, as it seeks to verify the successful opening of a dialog with specific text and expects the dialog to close upon clicking the close button. Despite that, I'm open to the notion of increasing the timeouts for these specific tests. I've also noticed some of these test cases failing in my local development setup but passing in the CircleCI ones. This discrepancy between the environments adds another layer of complexity to the situation. |
|
It would be a lot easier if it was consistent. I've had tests fail and then pass, only to fail again later. I think it is probably related to a greater usage of |
|
If that were the case, shouldn't a lot of these test cases fail? |
|
That would be intuitive. However, that is the only thing that I saw in common when I was spot checking the failing tests. I'll drop the EDIT: Nevermind. Both of the tests that failed when I dropped some |
tsmock
force-pushed
the
chore/frontend-dependency-cleanup
branch
from
dbec7f5
to
1ff6121
Compare
|
I noticed that the circle layer for the tasks centroid on the project detail page does not navigate to the task selection page when clicked. This functionality appears to be broken. EDIT: Something unusual is happening here: when you press the back button on the browser numerous times, you'll find that the app did navigate to the intended route, indicating that the on-click event is being triggered multiple times. We can eliminate the problem by removing the relative navigation and instead using the full path '/projects/:id/tasks,' but the back functionality still won't work. |
|
I checked on the As far as going to the wrong page, here is the calls that leads to the incorrect behavior:
The current state of code probably triggers the call navigate calls another 6 times. EDIT: I've made a PR for this at #5962. |
tsmock
force-pushed
the
chore/frontend-dependency-cleanup
branch
from
13def81
to
e9c741a
Compare
|
I think I'm going to start splitting some of the changes out. Specifically,
|
tsmock
force-pushed
the
chore/frontend-dependency-cleanup
branch
from
e9c741a
to
b90a6dc
Compare
quickstart.js has the same spacing as it had previously (which comes from the empty <p/>). Signed-off-by: Taylor Smock <tsmock@meta.com>
* `Sign Up` button * comment boxes * actionsForm.js (mostly buttons) Signed-off-by: Taylor Smock <tsmock@meta.com>
This reduces the main chunk size from 3.65 MB to 2.56 MB. Something about the commit "Use CRACO to workaround CRA #11770" caused several large components to be added to the main.*.js bundle. All suspense elements are *roughly* the same size as the expected text editor box, plus or minus a row. Signed-off-by: Taylor Smock <tsmock@meta.com>
* Avoid default imports in ReactJS There were quite a few files where we imported react, but didn't actually use it. * Use non-default imports where possible * Use named imports where possible Signed-off-by: Taylor Smock <tsmock@meta.com>
Signed-off-by: Taylor Smock <tsmock@meta.com>
Signed-off-by: Taylor Smock <tsmock@meta.com>
… handling Signed-off-by: Taylor Smock <tsmock@meta.com>
Signed-off-by: Taylor Smock <tsmock@meta.com>
Signed-off-by: Taylor Smock <tsmock@meta.com>
Signed-off-by: Taylor Smock <tsmock@meta.com>
…g to finish Signed-off-by: Taylor Smock <tsmock@meta.com>
Signed-off-by: Taylor Smock <tsmock@meta.com>
Signed-off-by: Taylor Smock <tsmock@meta.com>
This should fix some timeout issues.
Signed-off-by: Taylor Smock <tsmock@meta.com>
tsmock
force-pushed
the
chore/frontend-dependency-cleanup
branch
from
826ed49
to
67ad269
Compare
|
|
@ramyaragupathy I think we are good to go on this PR and we are ready to merge it into staging for now and will create another PR fixing the osm download most probably today and will let manjita test it. |
This PR intends to make the TM compatible with the defaults of Debian Bookworm. This means that the frontend needs to be compatible with NodeJS 18.x.
In general, the code changes are intended to be limited to compatibility changes. In the event that there needs to be a reformat due to formatting tool updates, those will be in separate commits (to make rebasing onto a new develop HEAD easier).
This PR replaces frontend PRs #5712, #5640, #5586, #5585, #5583, #5540, #5529, #5510, #5474, #5467, and #5177.
This does not update Rapid to v2 -- that will need to be a separate PR.
Total bundle size has decreased from 14.26 MB to 8.07 MB.
main bundle size has decreased from 3.12 MB to 2.56 MB.
At one point, I had the main bundle size reduced to 700 KB (before I updated the react-scripts to 5.0.1, which required a workaround for facebook/create-react-app#11770 ). Either that update or the workaround caused two bundles to merge; the first with our code, the second with most of the "heavy" modules.
Test runs from #5721
[1] bundleSizes.zip