Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Next v12 compatibility #294

Closed
wants to merge 6 commits into from

Conversation

jasonwilliams
Copy link
Collaborator

@jasonwilliams jasonwilliams commented Feb 23, 2022

What kind of change does this PR introduce?

Compatibility with next v12

There were 2 type issues, one was this:
vercel/next.js#28646
Which needed defaultGetInitialProps to be added.

The other resolved itself by having useMaybeDeferContent removed.

How to test this PR:

  • You will need to be on node v16+
  • You will need to be on Next v12+
  • npm i --save-dev next-page-tester/next-page-tester#pull/294/head
  • Set __NEXT_TEST_MODE=jest env

@jasonwilliams jasonwilliams changed the title v12 compatibility Next v12 compatibility Feb 23, 2022
This was referenced Feb 23, 2022
@jasonwilliams
Copy link
Collaborator Author

jasonwilliams commented Feb 24, 2022

change to use import() instead of require here: https://github.com/next-page-tester/next-page-tester/blob/master/src/loadFile.ts#L18

Which means await changes all over the place. Any help would be useful

"node": ">=v12.22.8"
will need bumping to v16 which is current LTS

Jason Williams added 3 commits February 25, 2022 14:37
So instead we need to check if its a function. If it is it has a default export, if not its most likely empty or no default export has been set.
@jasonwilliams
Copy link
Collaborator Author

Help is needed to run these tests and try to fix the failing ones. I think using import can help prevent the require issues. If not we may have to just move to vitest completely.

@toomuchdesign
Copy link
Collaborator

toomuchdesign commented Feb 25, 2022

Hi @jasonwilliams! If the one running here is the last version, I can see a segmentation error on Babel compilation.

The remote branch was probably outdated, segfault errors seem to be gone 👍 and tests hang during execution.

I noticed something which could be worth some consideration. I believe the async nature of the new import strategy could play not well with Jest's isolateMoudules which accepts only a sync callback. It could be the reason for the failure of some tests (especially the ones ensuring values consistency between server and client).

@jasonwilliams
Copy link
Collaborator Author

jasonwilliams commented Feb 26, 2022

The hanging tests have (kinda) been fixed.
I noticed this:
https://github.com/vercel/next.js/blob/canary/packages/next/server/config.ts#L644-L648

The segfault happens on line 650 (the import branch)
If you set the env var __NEXT_TEST_MODE to jest before running the tests they will now all pass.

As a "quick fix" we could just do this and call it a day, but we would end up stuck with jest, plus Next.js may not support the require branch forever. That being said we may not have any other choice. I can take a look at vitest but if vitest doesn't work we have a path forward

I noticed something which could be worth some consideration. I believe the async nature of the new import strategy could play not well with Jest's isolateMoudules which accepts only a sync callback. It could be the reason for the failure of some tests (especially the ones ensuring values consistency between server and client).

yes this has been a big problem. I've tried vitest but get some problems there also. I will give it another go now that we've removed babel. Maybe the isolatedModules plus require still being used was messing it up also

@jasonwilliams jasonwilliams mentioned this pull request Feb 26, 2022
2 tasks
@jasonwilliams
Copy link
Collaborator Author

jasonwilliams commented Mar 1, 2022

Forcing __NEXT_TEST_MODE to be 'jest' would fix this PR for all those who use Jest as a test runner. But i don't know the effects for Mocha etc..

@toomuchdesign
Copy link
Collaborator

Ehi @jasonwilliams, I'll answer here to the question you asked privately about using Jest's isolateModules to feed the discussion. That was introduced to simulate Next.js server and client renders in a single testing environment. It turned out to be quite necessary to any non-basic Next setup.

You can read more about it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants