Dynamically fetch control epic copy #9

tjmw · 2020-01-21T15:17:21Z

This PR introduces a change to dynamically fetch the control epic copy from a remote endpoint. This happens once when the app is started. There isn't an elegant failure handling mechanism at this point. We'll figure that out in a future PR. Currently we'll catch and log any errors which occur during this process.

Not implemented here

Elegant handling/retrying if we fail to fetch the content
We don't have full support for all possible placeholders yet (I think the full list is %%CURRENCY_SYMBOL%%, %%COUNTRY_NAME%% and %%ARTICLE_COUNT%%)
We do support interpolating the currency symbol, but we just use a hardcoded value. In future we'll want to bring across the logic from frontend where we resolve this based on the geolocation.

tjmw · 2020-01-21T15:18:59Z

src/components/DefaultEpic.tsx

+        {paragraphs.length > 0 &&
+            paragraphs.map((paragraph, idx) => (
+                <p key={idx} className={bodyStyles}>
+                    <span dangerouslySetInnerHTML={{ __html: replacePlaceholders(paragraph) }} />


I've introduced an additional span here around the paragraph content to allow me to use dangerouslySetInnerHtml, since I also want to include children below (the Highlighted component appears within the same <p> tag). I'm interested to hear thoughts on alternative approaches here.

Yeah this is annoying and I don't have any better solution I'm afraid :(.

I don't have a better solution either. But for the sake of argument, if we absolutely had to stop that extra span from being added, I suppose we could move the dangerouslySetInnerHTML call back to the paragraph level and ensure the HTML we pass into it includes the paragraph text, as well as the result of passing the Highlighted component through ReactDOM's renderToStaticMarkup function, which we do use further upstream. But again, I personally wouldn't do that in this case as we can live with that span.

nicl

This is fetch!

src/components/DefaultEpic.tsx

nicl · 2020-01-21T16:11:01Z

src/components/DefaultEpic.tsx

+        {paragraphs.length > 0 &&
+            paragraphs.map((paragraph, idx) => (
+                <p key={idx} className={bodyStyles}>
+                    <span dangerouslySetInnerHTML={{ __html: replacePlaceholders(paragraph) }} />


Yeah this is annoying and I don't have any better solution I'm afraid :(.

src/components/DefaultEpic.tsx

nicl · 2020-01-21T16:17:27Z

src/server.tsx

 import { DefaultEpic } from './components/DefaultEpic';
 import cors from 'cors';

-const app = express();
-app.use(express.json({ limit: '50mb' }));
+const bootApp = (content: DefaultEpicContent): void => {


What was the thinking behind not using top-level await in the end:? (Just curious really.) I've seen suggestions that it makes modules a bit harder to reason about/can be surprising, but on the flip-side for the key start file this might not matter and it might help remove the bootApp wrapping and indentation?

Good question! It was a combination of things. When I looked it up I was reminded that, as you mentioned here, there is some push-back on whether or not top level await makes sense. I don't think any of the arguments against it affect us right now, but felt like maybe it was setting a precedent and it could be easy to avoid us having the conversation in future.

The other (somewhat major 😸) reason is that it's not supported in TypeScript yet.

Ha okay, that last argument is pretty slam-dunk :).

It looks like they're adding it in the next version - https://devblogs.microsoft.com/typescript/announcing-typescript-3-8-beta/#top-level-await

tjmw · 2020-01-21T16:29:41Z

Thanks for the review & feedback @nicl! I'm either proud or ashamed to say that I didn't need to follow the link to get the Mean Girls reference 😝

andre1050

Great work @tjmw! 🆒💥

andre1050 · 2020-01-21T16:30:14Z

src/components/DefaultEpic.tsx

+
+const Body: React.FC<BodyProps> = ({ highlighted, paragraphs }: BodyProps) => (
+    <>
+        {paragraphs.length > 0 &&


Don't think we need to check for the length of the array before we map through it, do we? With a length of 0 the map callback would be called 0 times, right?

Ah thanks, good spot! I think at some point I had a version where I did want this, but definitely not needed now.

andre1050 · 2020-01-21T16:45:26Z

src/components/DefaultEpic.tsx

+        {paragraphs.length > 0 &&
+            paragraphs.map((paragraph, idx) => (
+                <p key={idx} className={bodyStyles}>
+                    <span dangerouslySetInnerHTML={{ __html: replacePlaceholders(paragraph) }} />


I don't have a better solution either. But for the sake of argument, if we absolutely had to stop that extra span from being added, I suppose we could move the dangerouslySetInnerHTML call back to the paragraph level and ensure the HTML we pass into it includes the paragraph text, as well as the result of passing the Highlighted component through ReactDOM's renderToStaticMarkup function, which we do use further upstream. But again, I personally wouldn't do that in this case as we can live with that span.

tjmw · 2020-01-21T16:53:13Z

Thanks for the feedback @andre1050! I couldn't respond directly to your comment about dangerouslySetInnerHTML for some reason, but yeah I agree with your thinking there. I'm happy to stick with it as-is and we can revisit if there's a strong reason we need to get rid of the additional <span>.

nicl · 2020-01-21T17:11:52Z

src/utils/fetchDefaultEpicContent.test.ts

@@ -0,0 +1,39 @@
+import { fetchDefaultEpicContent } from './fetchDefaultEpicContent';


cc @andre1050 and @tjmw - what are people's preferences on:

a) naming these files? (with Andre earlier I'd assumed we'd go for nouns as good names here - the reason being that it makes it easier to add additional functions later, i.e. they are more like mini-modules or objects, which typically have noun naming

b) utils folder? - I don't have strong views on this, generally I prefer a name other than utils though (or things like common - as common in Frontend still makes me sad).

Hey @nicl both good questions!

For a) yep I can see the point here. I agree that's the direction I'd expect the naming to go eventually. The name as it is now definitely doesn't invite adding additional functions. I guess the danger is going with something too generic before figuring out what we want this module to be which can end up being misleading. So we could rename now or put that decision off until we have a concrete use case. I don't feel too strongly either way.

b) yeah I think we should look to split utils up into several more specifically named folders. I've definitely seen utils become a bit of a dumping ground in projects in the past. Perhaps the file added here belongs in src/api or similar.

I ended up reorganizing and renaming things here a little bit. We've now got src/api/contributionsApi where we can perhaps group all API requests we need to make to fetch contributions data. Maybe not perfect, but we can always change in future.

Fetch the copy for the default epic from an API endpoint once when we boot the app. At this point we're still missing the following, which we'll follow up on in future PRs: * Elegant handling/retrying if we fail to fetch the content * We don't have full support for all possible placeholders yet (I think the full list is %%CURRENCY_SYMBOL%%, %%COUNTRY_NAME%% and %%ARTICLE_COUNT%%) * We do support interpolating the currency symbol, but we just use a hardcoded value. In future we'll want to bring across the logic from frontend where we resolve this based on the geolocation.

tjmw commented Jan 21, 2020

View reviewed changes

tjmw requested review from andre1050 and nicl January 21, 2020 15:19

nicl approved these changes Jan 21, 2020

View reviewed changes

andre1050 approved these changes Jan 21, 2020

View reviewed changes

tjmw force-pushed the feature/dynamic-epic-content branch from 661f514 to 0f2197e Compare January 21, 2020 17:00

nicl reviewed Jan 21, 2020

View reviewed changes

tjmw force-pushed the feature/dynamic-epic-content branch from 89566b9 to 0c34a4b Compare January 22, 2020 09:39

tjmw merged commit f312756 into master Jan 22, 2020

tjmw deleted the feature/dynamic-epic-content branch January 22, 2020 09:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamically fetch control epic copy #9

Dynamically fetch control epic copy #9

tjmw commented Jan 21, 2020

tjmw Jan 21, 2020

nicl Jan 21, 2020

andre1050 Jan 21, 2020

nicl left a comment

nicl Jan 21, 2020

nicl Jan 21, 2020

tjmw Jan 21, 2020

nicl Jan 21, 2020 •

edited

tjmw commented Jan 21, 2020

andre1050 left a comment

andre1050 Jan 21, 2020

tjmw Jan 21, 2020

andre1050 Jan 21, 2020

tjmw commented Jan 21, 2020

nicl Jan 21, 2020

tjmw Jan 21, 2020

tjmw Jan 22, 2020

		@@ -0,0 +1,39 @@
		import { fetchDefaultEpicContent } from './fetchDefaultEpicContent';

Dynamically fetch control epic copy #9

Dynamically fetch control epic copy #9

Conversation

tjmw commented Jan 21, 2020

Not implemented here

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicl Jan 21, 2020 • edited

Choose a reason for hiding this comment

tjmw commented Jan 21, 2020

andre1050 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjmw commented Jan 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicl Jan 21, 2020 •

edited