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

feature: require pages in browser and non-browser environment #73

Merged
merged 11 commits into from
Dec 28, 2020

Conversation

toomuchdesign
Copy link
Collaborator

@toomuchdesign toomuchdesign commented Dec 20, 2020

What kind of change does this PR introduce?

This PR tries to extend current server idea to workaround #67 and #64. The main issue here consists of being able to access the same module in both "browser" and "non-browser" environment. This approach tries to consider "browser" mode modules as default and keep a "non-browser" copy of the relevant modules in a server instance initialized when getPage gets called.

Relevant modules are:

  • pages
  • custom app/document
  • some Next.js modules (like next/dist/next-server/lib/side-effect)

What is the current behaviour?

You can also link to an open issue here.

What is the new behaviour?

Page components (including _app and _document) are imported twice: with and without browser's global objects. There available through pageObject.page.client and pageObject.page.server.

Client versions are imported with global require cache while server versions gets imported via an isolated require cache (using jest.isolatedModules or stealthy-reqruire).

Does this PR introduce a breaking change?

Yes, but the result should be closer to what is expected from an actual Next.js instance running on server + client.

Users might optionally and temporarily patching jest until Jest v27 is released (see docs).

Other information:

Please check if the PR fulfills these requirements:

  • Tests for the changes have been added
  • Docs have been added / updated


export function jestIsolateModules<T>(f: () => T) {
return new Promise((resolve, reject) => {
jest.isolateModules(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will probably have to check if jest is defined and have an approach in place that could work across different testing frameworks? Perhaps others do support manual modification of require.cache and it wont be that complicated anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, 100% agree. I'd love to keep this more or less framework agnostic.

This means providing a isolateModules equivalent that works with vanilla require. By now I found a few packages to bypass require's cache, but they don't consider nested requires:

We might search better or try our own implementation (if feasible). Still we wouldn't be able to support out of the box other test frameworks which provide their own require reimplementation.

In order for the strategy taken in this PR to succeed, we need to wait until jest publishes its next release with fixed isolatedModules.

In the meanwhile I might try to patch Jest with patch-package.

@coveralls
Copy link

coveralls commented Dec 23, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling abbd209 on server-pages into ceb3ebc on master.

@toomuchdesign toomuchdesign force-pushed the server-pages branch 3 times, most recently from baf3e9d to 21f2e23 Compare December 23, 2020 12:13
@toomuchdesign
Copy link
Collaborator Author

toomuchdesign commented Dec 23, 2020

Hey @Meemaw! I surprisingly got to get some outcome from this PR. There are quite a few open points to be more carefully considered and I'm sure you've a lot to bring into the discussion.

What this PR does

This PR enhances the way page files are required runtime. Each page is required twice:

  • once normally while JSDOM is available
  • once without JSDOM's global object (and bypassing require's cache to get fresh modules)

The 2 page files are available through PageObject which now looks like:

type PageObject = {
-  page: NextPageFile;
+  page: {
+    client: NextPageFile;
+    server: NextPageFile;
+  }
  route: string;
  //...
};

The same happens for custom App and Document, required in both client and server versions.

jest.isolateModules vs. jest.resetModules

In order to retrieve both file versions, client file is imported normally, and server one is imported via jest.isolateModules.

jest.isolateModules has the advantage of not having side effects BUT it's a feature only available with jest. I can't currently see how we could replicate jest.isolateModules in a vanilla node environment.

Therefore, this PR means tying next-page-tester to jest. Unless we find a solution :)

jest.resetModules would be replicable in a vanilla node environment, but we have to find out whether we can work around its side-effects.

Waiting for Jest'ot

This PR also depends on JEST publishing this commit.

Notes

  • Custom document uses client version of page file (server version currently blows up everything)
  • server page file versions are mainly used for data fetching methods
  • We directly return the DOM tree as should be rendered on the client (skipping the initial DOM tree rendered server-side and returned as HTML at first request)
  • I would have liked to get rid of this, but I haven't found how, yet.

@Meemaw
Copy link
Collaborator

Meemaw commented Dec 23, 2020

This looks very promising @toomuchdesign 🚀

A few comments:

  • Could we work with require.cache directly to "replace" jest.isolateModules in native node env? e.g. after requiring "sever" version we would clear the cache, and client would get fresh environment?
  • I would add some more tests that are more closely aligned to what users will use. I was thinking about removing real-world-example and introduce a folder for testing some well known libraries that rely on this behaviour (this was the initial idea of real-world-example. Had something like this in mind:
    • /third-party-integrations/react-hook-form
    • /thiry-party-integrations/aws-amplify
    • ...

@toomuchdesign
Copy link
Collaborator Author

toomuchdesign commented Dec 23, 2020

Based on what I know about require, we could replicate jest.isolateModules for the specific module required, but I currently don't see clearly how we could make it recursive to get fresh instances of the modules imported by the root one.

I was thinking of appending a flag to require object to signal "give me fresh modules" (shame on me 🙈), but I'm not even sure whether require figures as an actual object in Node context. :) Let's find out.

As for transforming real-world-example into a library-specific thiry-party-integrations tests folder seems definitely a wise idea. 👍

@toomuchdesign
Copy link
Collaborator Author

I found out that bypassing require.cache -even for deep requires- it's an almost no-brainer. Once I did it, I found stealthy-require 😍 and opted for it.

Currently we're manually checking for typeof jest !== 'undefined' and using jest.isolateModules or stealthy-require.

There's one dilemma left: how do we test it?? Shall we use another test runner like ava and run a test case with it after/before jest?

@Meemaw
Copy link
Collaborator

Meemaw commented Dec 23, 2020

Yes, if possible, it would be ideal to run the whole test suite with another test runner, e.g. ava and mocha. I think syntax for test cases is the same so it should work out of the box? The difference would probably be a different setup file, but I'm not that familiar with other test runners.

@toomuchdesign toomuchdesign marked this pull request as ready for review December 24, 2020 10:22
@toomuchdesign toomuchdesign changed the title Server pages feature: require pages in browser and non-browser environment Dec 26, 2020
@toomuchdesign toomuchdesign linked an issue Dec 26, 2020 that may be closed by this pull request
@toomuchdesign
Copy link
Collaborator Author

I though we might consider publishing this PR even before Jest gets published.

On our side we've everything in place to support upcoming Jest version and (optimistically) any other test runner. On top of this, the issue addressed by this PR should affect a small part of users.

Users might be still able to get the features provided by this PR by patching Jest, the same way we do. I added a note in the readme about it.

As a downside, this would mean our users should remember to remove the patch when they'll update to Jest v27.

How do you see it @Meemaw? Feasible or too risky? PS. happy holidays :)

@Meemaw
Copy link
Collaborator

Meemaw commented Dec 28, 2020

Hey 🎄 I think we should merge this PR as it unlocks some nice things (next/config,...) but delay the release for some time and release it some time after the New Year's? I don't think a lot of people will be upgrading these days 🙂 Moreover, I have some follow up PRs in mind that will "stress test" the implementation (using https://github.com/styletron/styletron with SSR which did not work previously and will still require some fixes I believe)

@toomuchdesign
Copy link
Collaborator Author

Perfect! Let's merge, then. Take your time to stress this lib as much as you like. 🙌

@toomuchdesign toomuchdesign merged commit 0d63fc4 into master Dec 28, 2020
@toomuchdesign toomuchdesign deleted the server-pages branch December 28, 2020 11:22
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.

Usage with @aws-amplify next/SSR
3 participants