Core refactor part 2 #1057
Core refactor part 2 #1057
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1057 +/- ##
==========================================
- Coverage 83.19% 83.10% -0.10%
==========================================
Files 74 79 +5
Lines 3053 3012 -41
Branches 867 839 -28
==========================================
- Hits 2540 2503 -37
+ Misses 460 456 -4
Partials 53 53
Continue to review full report at Codecov.
|
jvarho
force-pushed
the
core-refactor-part-2
branch
from
90e8f39
to
845b29b
Compare
|
After these latest commits all the tests pass and lib/core has >99% test coverage. Coverage only falls because this removes lines overall. Some e2e tests fail to build, but I can't see how it's because of any changes I've made when the other jobs go fine. I will continue with the next steps, but this can be merged unless one of the changes mentioned above is a problem. |
The yarn build step failed here but the workflow still continued, not sure why. The actual problem looks to be with the yarn cache: Usually rerun will fix it but GH Actions doesn't support just re-running a single job right now =( |
jvarho
force-pushed
the
core-refactor-part-2
branch
2 times, most recently
from
c40d5a0
to
1cbcf6c
Compare
jvarho
force-pushed
the
core-refactor-part-2
branch
from
1cbcf6c
to
2589491
Compare
|
Now the tests got far enough to show an actual bug somewhere: https://github.com/serverless-nextjs/serverless-next.js/runs/2555973177?check_suite_focus=true#step:9:1104 Looks like perhaps vercel/next.js#21943 which a nextjs update in the relevant e2e tests would fix. I should have time to look into it tomorrow. |
|
Had time sooner after all. My interpretation, based on testing local deploy with a couple of versions and logging statements:
For testing purposes I decided to upgrade one of the two locale tests to 10.0.9 and one to 10.2.0, so those should now pass. Might be better to decide which versions are supported, spell it out in README and upgrade all the e2e tests to the latest versions of those major.minor releases? |
Previously I was trying to support the latest two minor versions, so when a new version of Next.js is released, I manually copied the current e2e tests over with |
That makes a lot of sense. I don't think two versions should be hard to support – except for the latest features of course. But I was unsure what the target was – the readme starts off by talking about version 9.0... |
|
Yup we can update those since it’s next.js 10 now :)
…On Tue, May 11, 2021 at 10:51 AM Jan Varho ***@***.***> wrote:
Previously I was trying to support the latest two minor versions, so when
a new version of Next.js is released, I manually copied the current e2e
tests over with prev- prefixed to them and then bumped the current
versions up to the latest. So that way we could at least have a bit of
backwards compatibility (some people). But I guess it makes it harder to
maintain as well...
That makes a lot of sense. I don't think two versions should be hard to
support – except for the latest features of course. But I was unsure what
the target was – the readme starts off by talking about version 9.0...
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1057 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAYSQFA7OZCVYWOQANLAML3TNFVAVANCNFSM44OSFN4Q>
.
|
|
Eh, back to the drawing board I suppose. The tests I looked at in detail were fixed by the version upgrades, but others are still failing. |
|
Ok, so upgrading the next.js versions fixed one issue, but uncovered another. There is a front end change that drops the default locale under some situations. I've reported the inconsistent behavior as a bug in next.js, but there are many locale related open issues, so I guess that functionality is still a bit in flux. I modified the tests to pass with the current behavior, not sure what the best solution is. An api rewrite test fail was an actual bug in my code, which I fixed. So even though the e2e tests are annoying to get to run, they are useful. |
Thanks for fixing the tests. Yeah, I don't personally use the locales feature too much, so not sure what is the best either. If you can make it work with the latest version, that is probably the best compromise for now. If users have issues with the latest of this component, they should probably try to upgrade Next.js first. |
Moves most of the request routing over to core.
I had to make test changes as well due to:
There were simply tests where the test data was wrong, but the old logic worked differently and happened to work.
The 404 logic was tested two ways (render and serve) with the same manifests. I dropped the non-static 404 tests. I hope that's ok, AFAIU Next.js will always generate a static 404 since at least 9.5. (Note: static 500 logic should be implemented at some point.)
Rewrites had "no-op rewrite" support (Dynamic rewrites 404ing #733) which I was not able to reproduce with Next.js server and dropped. Those add complexity, but if we want to diverge from Next.js I can try to reimplement them. However, with the new types of rewrites added in 10.2 that may not be possible consistently.
Rewriting query parameters worked inconsistently when locale support was/not used.
I did not yet fix the routing priority issues I found, but they are flagged in a couple of comments. This branch reproduces the same (buggy) logic of going through routes by type rather than specificity. Before fixing that the build logic should be refactored, as doing the correct thing efficiently will require manifest changes.
Next steps:
Unrelated TODO: