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

Fail when CORS proxy URL is required but missing #101

Closed gomorizsolt closed 4 years ago

gomorizsolt commented 4 years ago

As is: if the option is omitted then getProxyURL() will produce:

undefined/https://medium.com/feed/c-hive

Expected:

As a minor improvement, let's also move getProxyURL() to its relevant context since it's not reused elsewhere.

thisismydesign commented 4 years ago

Let's raise an error if the proxy is not configured for services that need it.

thisismydesign commented 4 years ago

Is this still present?

gomorizsolt commented 4 years ago

AFAICT yes, it is.

thisismydesign commented 4 years ago

Giving a higher priority as it blocks open-sourcing - good point btw ;)

thisismydesign commented 4 years ago

Let's raise an error if the proxy is not configured for services that need it.

Just to point, if the proxy is not configured but there're no components that'd use it that's OK, no need to raise.

gomorizsolt commented 4 years ago

Let's raise an error if the proxy is not configured for services that need it.

Just to point, if the proxy is not configured but there're no components that'd use it that's OK, no need to raise.

Thanks. One question though: should this "crash" the whole page (i.e. throw new Error() without catching it somewhere) or let's just leave a message in place of the component where it's required? I'm of the opinion that this shouldn't affect the rendering of other components (see https://github.com/c-hive/dotdev/pull/112/files#r417883310).

thisismydesign commented 4 years ago

IMO it's ok if the page crashes in this case. Just make sure the error is visible in the console. But if you already created a smarter solution let's use that.

gomorizsolt commented 4 years ago

Resolved by #120.