Support new content version release in codebase #4441

corwintines · 2021-11-10T19:53:37Z

Description

Changes to gatsby-node to parse markdown and json files for translation.

With JSON we now check:

if translation file exists
If there are the same amount of keys
If all the keys are the same

With Markdown we now check:

The parsed info structure of the mdx

Related Issue

…check after team discussion

samajammin · 2021-11-16T03:45:06Z

This looks great at a glance @corwintines! I still need to review this in depth. Will do so tomorrow...

One quick thought since we're using the header ids to compare markdown files - we should ensure we have header ids generated for all our markdown files. i.e. please run this script on all English files: https://github.com/ethereum/ethereum-org-website/blob/dev/src/scripts/generate-heading-ids.js

I think ideally we run this as a pre-commit hook on any English file but I'm ok with saving that for a later issue.

pettinarip

Great work @corwintines. I have added a couple of comments to refactor the code a bit.

gatsby-node.js

pettinarip · 2021-11-17T15:13:23Z

gatsby-node.js

+      }
+    }
+  } catch {
+    return true


I don't like this return that is absorbing all the errors and is different from the return type from the function.

Let the function throw its errors and that the consumer handles what to do with it.

pettinarip · 2021-11-17T15:19:54Z

gatsby-node.js

-exports.onCreateNode = ({ node, getNode, actions }) => {
+// Loops through all the files dictated by Gatsby (building pages folder), as well as
+// folders flagged through the gatsby-source-filesystem plugin in gatsby-config
+// This seems to be where the source and transform holdup is?


I would suggest removing the comments with questions from the code. I think we can discuss them off record than leaving them there.

And regarding the question, hmm not sure. I think it is more related with the queries execution, when gatsby is preparing to build the pages.

gatsby-node.js

samajammin · 2021-11-17T19:37:10Z

gatsby-node.js

+    return true
+  }
+}
+
 const outdatedMarkdownPages = [


I don't believe outdatedMarkdownPages are used anymore? ✂️

Co-authored-by: Sam Richards <sbrichards@gmail.com>

gatsby-node.js

samajammin · 2021-11-17T21:51:32Z

gatsby-node.js

+  try {
+    const splitPath = path.split("/")
+    splitPath.splice(7, 2)
+    const englishPath = splitPath.join("/")


I'm not sure this is right - don't you need to replace the lang (e.g. ar) with en in the path?

I think as-is this only drops the /content/translations/ from the path.

I need to parse out without __dirname I think to get this right in hindsight. Will add that in.

samajammin · 2021-11-17T22:07:12Z

gatsby-node.js

-        isContentEnglish:
-          langVersion < 2 && !page.component.includes("/index.js"),
+        isContentEnglish,
+        // // We can check if isContentEnglish through our crawling


Also I believe langVersion is no longer used, so can delete that variable.

samajammin · 2021-11-17T22:14:27Z

General note on this refactor - this now removes the concept of version for each language (since it's now defined on a page-by-page basis), so I believe we can delete that key for each language here: https://github.com/ethereum/ethereum-org-website/blob/dev/src/data/translations.json

This also means logic that relies on that should be removed/refactored, e.g. any use of getLangContentVersion. We'll also have to sort out how to rework hasTutorials - maybe we just look at the file system to see if there's any tutorials that exist for that language (i.e. does the directory src/content/translations/{lang}/developers/tutorials exist).

Edit: as a result of this we'll also need to rework how we do the contentV1Pages & contentV2Pages in gatsby-node.js, since that relies on getLangContentVersion. I think we could take a similar approach: inspect the file system to see if those markdown pages exist & if not, create the component page.

gatsby-node.js

samajammin · 2021-11-19T20:34:34Z

gatsby-node.js

 // Add additional context to translated pages
 // https://www.gatsbyjs.com/docs/creating-and-modifying-pages/#pass-context-to-pages
-exports.onCreatePage = ({ page, actions }) => {
+exports.onCreatePage = async ({ page, actions }) => {
  const { createPage, deletePage } = actions

  const isTranslated = page.context.language !== defaultLanguage
  const hasNoContext = page.context.isOutdated === undefined
  const langVersion = getLangContentVersion(page.context.language)


We're going to need to re-think how we do this, since version is no longer on a language level.

samajammin · 2021-11-24T04:03:38Z

src/pages/developers/tutorials.js

-        ? tutorial.lang === pageContext.language
-        : tutorial.lang === "en"
-    )
+    .filter((tutorial) => tutorial.lang === pageContext.language)


I don't think this logic quite gives us what we want. If a language doesn't have any tutorials, we'll now display 0 tutorials for that language, e.g.

What we want is to:

if the language has translated tutorials, display those

if the language doesn't have translated tutorials, display our English tutorials

I feel like we discussed just refactoring the hasTutorials to be a helper function in this file that iterates through the tutorials & to return true as soon as it hits one in that native language. If it doesn't find one, return false. Basically lodash's any or some: https://lodash.com/docs/4.17.15#some

(we have lodash installed in the project, so you should be able to use it).

Ah gotcha, my mistake. Will get that added in now!

samajammin · 2021-11-24T04:10:31Z

gatsby-node.js

+      const markdownPath = `${__dirname}/src/content/translations/${lang}/${page}/index.md`
+      const langHasOutdatedMarkdown = fs.existsSync(
+        markdownPath,
+        fs.constants.R_OK | fs.constants.W_OK


Is this 2nd param needed? What's it do? I notice we don't use it in fs.existsSync elsewhere.

Probably dont need the W_OK part since we arent writing to it, but just checking if we can read the file. I am using existsSync to check if the file exists is all.

src/pages/index.js

gatsby-cloud · 2021-12-01T18:44:08Z

Gatsby Cloud Build Report

🚩 Your build failed. See the build logs here

samajammin · 2021-12-01T21:13:25Z

src/pages/index.js

    return <LegacyPageHome />
  }
+  console.log(intl)


src/content/developers/tutorials/how-to-use-tellor-as-your-oracle/index.md

samajammin · 2021-12-02T00:52:09Z

I think we might have to add some additional logic for the tutorial page. I believe current logic will always show the outdated banner? (we don't list every single tutorial on CrowdIn as we also have external tutorials)

FYI @minimalsm I don't think this is actually an issue. The tutorials homepage is a React component, so we don't compare header ids. As long as a language has all the matching keys from the JSON file (https://github.com/ethereum/ethereum-org-website/blob/dev/src/intl/en/page-developers-tutorials.json) it shouldn't display an outdated banner.

samajammin

This is looking super close!

Only issues I noticed were some markdown files (seemingly only the developer docs & developer tutorial pages) had wonky header ids (i.e. the header id didn't match the header text). The other markdown pages (e.g. defi & NFTs) seemed to be fine. I suspect because the script to generate the header ids may not work on deep directories?

Maybe we don't worry about refactoring that script but let's update those header ids to be sensible matches (maybe try running it on the specific files?).

samajammin · 2021-12-02T04:31:22Z

src/pages/developers/tutorials.js

-    .concat(externalTutorials, internalTutorials)
+  const allTutorials = [].concat(externalTutorials, internalTutorials)
+
+  const hasTutorialsCheck = some(allTutorials, ["lang", pageContext.language])


Perhaps more clear as langHasTutorials?

pettinarip

Looks great! @corwintines good job

pettinarip · 2021-12-02T16:51:18Z

@corwintines building it in my local, I see a long list of errors which I don't think adds too much value:

should we silence this? or in case we keep it, should we add more details on the error message (e.g. file paths being read)?

samajammin · 2021-12-02T16:55:01Z

Nice catch @pettinarip

should we silence this? or in case we keep it, should we add more details on the error message (e.g. file paths being read)?

I think the latter - I'm curious why we're seeing those errors.

pettinarip · 2021-12-02T17:01:00Z

I think the latter - I'm curious why we're seeing those errors.

I think we are mixing things, we are throwing an error on a possible function flow (from the function docs: If there is not english file, return true). I mean, that is not an error, that is an actual flow of the function.

corwintines · 2021-12-02T17:22:19Z

I was originally just returning true for these, but it was brought up that we wanted to throw the errors so I console logged them. I believe at the end of the day it just doesnt have access to a file and throws the error from fs.

samajammin · 2021-12-02T18:08:07Z

@corwintines & I dug in - we can remove the fs.readFileSync error.

feat: initial pass on new translation support. Will add new markdown …

52b15b1

…check after team discussion

github-actions bot added the internal 🏠 label Nov 10, 2021

feat: parse markdown for isOutdated based on info structure

84a9b7e

corwintines marked this pull request as ready for review November 15, 2021 21:37

corwintines requested review from minimalsm, pettinarip, samajammin and wackerow as code owners November 15, 2021 21:37

pettinarip requested changes Nov 17, 2021

View reviewed changes

pettinarip reviewed Nov 17, 2021

View reviewed changes