c-hive / dotdev

Next.js SPA professional website template for teams and individuals: https://c-hive.github.io/dotdev/
MIT License
0 stars 1 forks source link

Application error when newly opening the app #128

Closed thisismydesign closed 4 years ago

thisismydesign commented 4 years ago
gomorizsolt commented 4 years ago

I'm pretty sure the issue stems from:

https://github.com/c-hive/dotdev/blob/7cbc91181af1f45b55e1a6edc6a3fb4ad344d776/src/components/Projects/ProjectDisplayer/ProjectDisplayer.js#L52-L75

Let's suppose there's an error while fetching the languages for a certain repo:

Proposals:

(1)

if (languagesErr || !languages) {
   return null;
}

if (display === "icon") { ... }

// ...

(2)

Let's make useFetch() so that the components can pass off an initial value if needed:

const useFetch = (fetchFn, initialData) => {
   const [response, setResponse] = useState({
      data: initialData,
      // ...
   });
};

This way even if there's an error while fetching the languages the page won't crash.

I'd genuinely go with both because the mixture of the two seems to be a flexible and solid solution (but I'd say (1) is fine for now).

thisismydesign commented 4 years ago

Let's suppose there's an error while fetching the languages for a certain repo:

  • an informal warning is logged to the console

I didn't get this log though.

gomorizsolt commented 4 years ago

I don't know what's the problem with that but see https://github.com/c-hive/dotdev/pull/130/commits/de33b7dce3b887563b1ac3bea022c2943356820e.

gomorizsolt commented 4 years ago

New error after merging #129 and #130 (only in incognito mode):

Error: ConfigContext cannot be used outside the provider.

I don't get this error though. My gut feeling is that such errors have to do something with the mixture of server-side rendering and the rehydration process.

gomorizsolt commented 4 years ago

My gut feeling is that such errors have to do something with the mixture of server-side rendering and the rehydration process.

Alright, apparently I'm wrong. There's no server-side rendering involved whatsoever.

https://nextjs.org/docs/basic-features/data-fetching#fetching-data-on-the-client-side

Statically generated pages are still reactive: Next.js will hydrate your application client-side to give it full interactivity.

It's a static site that's turned into a fully interactive client-side app after the rehydration:

One of the main benefits of this feature is that optimized pages require no server-side computation, and can be instantly streamed to the end-user from multiple CDN locations. The result is an ultra fast loading experience for your users.

The caveats chapter, however, says that:

If you have a custom App with getInitialProps then this optimization will be turned off for all pages.

https://github.com/c-hive/dotdev/blob/31aed91aedd8c447332742ba724e0df697654797/pages/_app.js#L7

So it's eventually a server-side rendered app. In this case, we should make sure the rehydrated app matches the original (i.e. server-side rendered) HTML.

https://joshwcomeau.com/react/the-perils-of-rehydration/

This article points out why it may have a significant impact on the app and also proposes a component/custom hook to ensure dynamic data is only rendered/initiated when the app has rehydrated (i.e. turned into an interactive client-side app).

Note that I'm not 100% sure it'll fix the error(s) but I believe it's a practice that's worth keeping in mind when working with SSR/SSG.

gomorizsolt commented 4 years ago

One another thing I'd like to note is that it's hard to debug such errors. Gatsby (and I assume NextJS as well) opts out of server-side rendering for a short feedback loop to quickly see the changes.

This is a trade-off. By opting out of server-side-rendering in dev, Gatsby is optimizing for a short feedback loop. Being able to quickly see the changes you make is so, so important. Gatsby prioritizes speed over accuracy.

The general advice is being mindful of server-side rendering rehydration when working with these frameworks. The other approach could be to create a fork of the repo to be able to make sure of the changes once they're deployed.

thisismydesign commented 4 years ago

Let's stick to fixing this specific issue. Either investigate if others faced the same or try to see when it was introduced. Can it be reproduced locally? If it's only guessing and there's no way to reproduce / identify the issue then let's pt it on hold for now.

gomorizsolt commented 4 years ago

Can it be reproduced locally?

That's what I tried to point out with "it's hard to debug such errors". I've rather created a fork of the repo to make sure whether the local changes fix the deployed version as well.

thisismydesign commented 4 years ago

So neither the local dev version nor the local built version produces the same issue?

gomorizsolt commented 4 years ago

No, the local dev version (even if it's running in incognito) doesn't make the error reproducible. On the other hand, building the app and opening in incognito produces the same error as before:

TypeError: Cannot read property 'map' of undefined

But it's completely different compared to the recently reported error.

thisismydesign commented 4 years ago

That is possibly due to missing config, no?

gomorizsolt commented 4 years ago

No, the file exists locally (I mean the config file that's being ignored).

thisismydesign commented 4 years ago

When you run the build version doesn't that count as production env? So the config should come from env instead of file. Anyways, if this is a separate issue please create a new issue for it in the backlog.

thisismydesign commented 4 years ago

@gomorizsolt Fixed by https://github.com/c-hive/dotdev/pull/140 & https://github.com/c-hive/dotdev/pull/138?

gomorizsolt commented 4 years ago

Yep, thanks! It's fixed now.