Project app: use getServerSideProps on all project pages #1823

eatyourgreens · 2020-09-24T15:36:19Z

This removes getInitialProps from the project app and replaces it with getServerSideProps for each page. Common props (the project, theme and banner settings) are moved into a getDefaultPageProps helper.

I think you'll need to test against a production build in order to use server-side props without building the page on each request so this might best be tested with:

yarn build
yarn start

Package:
app-project

Review Checklist

General

Are the tests passing locally and on Travis?
Is the documentation up to date?

Components

Has a storybook story been created or updated?
Is the component accessible?
- Can it be used with a screen reader? BBC guide to testing with VoiceOver
- Can it be used from the keyboard? WebAIM guide to keyboard testing
- Is it passing accessibility checks in the storybook?

Apps

Does it work in all major browsers: Firefox, Chrome, Edge, Safari?
Does it work on mobile?
Can you yarn panic && yarn bootstrap or docker-compose up --build and app works as expected?

Publishing

Is the changelog updated?
Are the dependencies updated for apps and libraries that are using the newly published library?

Post-merging

Did the app deploy to https://frontend.preview.zooniverse.org/projects/:project-name/:owner or https://frontend.preview.zooniverse.org/about?
Is the new feature working or bug now fixed?
- Is there a Talk or blog post written to announce the new feature(s)?
Is the design working across browsers (Firefox, Chrome, Edge, Safari) and mobile?
- Is this approved by our designer?
Is this ready for production deployment?

eatyourgreens · 2020-10-22T10:26:47Z

This can't be deployed until #1826 is fixed in production, so I'm marking it as draft.

eatyourgreens · 2020-12-10T16:52:21Z

Removing draft status because we'll need this for the Engaging Crowds subject pages (see #1875.)

shaunanoordin

Approve

This PR consolidates the getServerSideProps() function, which has hitherto been initialised in the parent _app and then scattered across several sub-pages (e.g. /projects/foo/bar/classify, /projects/foo/bar/classify/workflow/1234, etc)

👍 Code changes look good to me; this seems to be mostly a refactor which will be relevant as we add more sub-pages.
Testing using yarn build ; yarn start works fine in most scenarios
❌❓ Testing using yarn dev, however, brings up some question marks.
- Uh, see the Dev Notes for more deets. Specifically, Test 1b and Test 2
❔ Functionality: I couldn't figure out how I'd break this code, so this is... good?
- I get the impression that the biggest thing the props-getting code does, at the moment, is getting the cookie values for "announcement banner dismissed?" and "light/dark mode?"
- However, this functionality doesn't seem to work even on master, so I don't have a functionality baseline to compare this branch with, so I'm ignoring functionality tests.

TL;DR: code changes look good, but what is up with yarn dev?

Dev Notes

Tested using macOS+Chrome

Test 1

branch: project-initial-props
urls:

http://localhost:3000/projects/darkeshard/engaging-the-crowds-2020
http://localhost:3000/projects/darkeshard/engaging-the-crowds-2020/classify
http://localhost:3000/projects/darkeshard/engaging-the-crowds-2020/classify/workflow/15764

Test 1a

command: yarn build ; yarn start
results:

👍 pages loaded & rendered fine. Classifier functions normally.
⚠️ switching to Dark/Light Theme isn't saved to cookies?
- But this is true for master also, so shrug? 🤷‍♂️

Test 1b

command: yarn dev
results:

❌ the pages keep spitting out errors
- engaging-the-crowds-2020 = "Server Error. Error: Error serializing .initialState.project.error returned from getServerSideProps in "/projects/[owner]/[project]". Reason: object ("[object Error]") cannot be serialized as JSOΩN. Please only return JSON serializable data types."
- engaging-the-crowds-2020/classify = "Server Error. Error: Error serializing .subjectSetID returned from getServerSideProps in "/projects/[owner]/[project]/classify". Reason: undefined cannot be serialized as JSON. Please use null or omit this value all together."
- _engaging-the-crowds-2020/classify/workflow/15764 = "Server Error. Error: Error serializing .subjectSetID returned from getServerSideProps in "/projects/[owner]/[project]/classify/workflow/[workflowID]". Reason: undefined cannot be serialized as JSON. Please use null or omit this value all together."
❓ BUT, this is ALSO true for master. The error on master, for the Classifier page, for the Engaging Crowds project is There was an error in the classifier :(
- If you look below at Test 2, I Fancy Cats has no similar issues on master, but does have problems on this branch.

Test 2

command: yarn dev
url: http://localhost:3000/projects/brooke/i-fancy-cats/classify I FANCY CATS

branch: master
results: 🆗

branch: project-initial-props
results: ❌ error: "Server Error. Error: Error serializing .subjectSetID returned from getServerSideProps in "/projects/[owner]/[project]/classify". Reason: undefined cannot be serialized as JSON. Please use null or omit this value all together."

Status

I wanted to give this a 👍 since the code changes/refactor looks good, but the extended testing with yarn dev made me scratch my head. 😖 There are a few problems that only trigger at the specific combination of this branch + yarn dev, while other problems are ongoing issues with master, and it's a bit hard for me to distinguish which are specific to this PR.

Change Request:

@eatyourgreens can you please try checking if your projects run fine with yarn dev? If they're OK, I'm going to give this PR an approval and run some follow up tests on master instead.

eatyourgreens · 2021-01-06T10:52:32Z

Thanks for the thorough review @shaunanoordin. Undefined values aren't valid JSON, and page props are serialised to JSON in the latest versions of NextJS (since 9.3 I think.) If you switch between the home page and classify page, and back, you should see the browser making requests for the page props as JSON on each client-side page transition.

Interesting that this works in a production build but fails in development. I think I'll have to filter out undefined values when building the page props, or give each page its own getServerSideProps function.

eatyourgreens · 2021-01-06T11:56:27Z

engaging-the-crowds-2020 = "Server Error. Error: Error serializing .initialState.project.error returned from getServerSideProps in "/projects/[owner]/[project]". Reason: object ("[object Error]") cannot be serialized as JSOΩN. Please only return JSON serializable data types."

I think I'm going to have to dive into how API errors are being handled in the project store. This sounds like the JavaScript error object is being stored, but can't be serialised. The initialState prop is a snapshot, which is used to hydrate the store in the browser. Here, the project has failed to load and that's broken the snapshot.

eatyourgreens · 2021-01-06T13:57:05Z

There's a property in the Engaging Crowds 2020 project which breaks the project model. I think this might be a separate issue, but I'm not sure. This is what the server logs when I load that project's home page.

Initialising Sentry: undefined production 3d64b7b949ae0fb1daca40aec410f1e73153f01a
Error loading project: Error: [mobx-state-tree] Error while converting `null` to `string`:

EDIT: I refined the model's error handling to catch invalid properties and found this bug. This will be broken on master, but I can fix it here.

Initialising Sentry: undefined production 3d64b7b949ae0fb1daca40aec410f1e73153f01a
project.researcher_quote is invalid Error: [mobx-state-tree] Error while converting `null` to `string`:

shaunanoordin · 2021-01-06T14:28:38Z

Thanks for confirming, @eatyourgreens ! If it looks like the problem is specific to Engaging Crowds 2020, we'll isolate investigations to that project.

I'm going to run one last test with with I Fancy Cats (post recent update), then I'll pop an approval off.

eatyourgreens · 2021-01-06T14:38:19Z

TIL: giving workflowID a default value of null breaks the classifier.

I've added conditional checks before assigning workflowID and subjectSetID props to the Classify page.

shaunanoordin · 2021-01-06T14:56:17Z

Ahh I was waiting for 4d40629 ! Yeah, I traced some problems in I Fancy Cats to this specific undefined-value-assignment as well. One sec, re-pulling the branch...

eatyourgreens · 2021-01-06T14:58:33Z

All project use /classify for the default workflow, so the classifier must be using that if a workflow ID isn't present in the URL.

shaunanoordin · 2021-01-06T15:53:35Z

I need to check something - I'm looking at commit ad7bf1d , and in this line...

// getServerSideProps.js
props.workflowID = project.configuration['default_workflow']

where is project coming from?

eatyourgreens · 2021-01-06T15:55:21Z

D'oh! One moment while I fix that.

shaunanoordin · 2021-01-06T17:40:00Z

We're almost there Jim! I think there's only one last possible glitch to worry about.

In this bit of code..

  if (workflowID) {
    props.workflowID = workflowID
  } else {
    props.workflowID = project?.configuration['default_workflow']
  }

// This works for for `yarn build ; yarn start`, but breaks for `yarn dev`, on I Fancy Cats

I'd recommend that it be changed to...

  if (workflowID) {
    props.workflowID = workflowID
  } else if (project?.configuration['default_workflow']) {
    props.workflowID = project?.configuration['default_workflow']
  }

// This works for `yarn dev`, on I Fancy Cats. Gonna test for `yarn build ; yarn start` next.

I think this is because the else clause works against the initial if (workflow) check. The else clause ensures that no matter what the props will have a workflowID, and if there's no project, then that's props.workflowID = undefined

eatyourgreens · 2021-01-06T17:44:28Z

If there’s no project, I want the workflow ID to be undefined. At least, I only want you to be able to classify if there’s a project and workflow.

shaunanoordin

PR Review (Update)

Changes are looking good, with only one suggested change remaining. I think the key takeaway here is that the system super duper hates it if getServerSideProps() returns anything with { someProperty: undefined }

Testing

I've streamlined my testing to use only the prototypical I Fancy Cats project, so I can sidestep the project-specific problems I encountered in the earlier review.

command: yarn build ; yarn start
url: http://localhost:3000/projects/brooke/i-fancy-cats?env=staging
result: 👍

command: yarn build ; yarn start
url: http://localhost:3000/projects/brooke/i-fancy-cats/classify?env=staging
result: 👍

command: yarn dev
url: http://localhost:3000/projects/brooke/i-fancy-cats
result: 👍

command: yarn dev
url: http://localhost:3000/projects/brooke/i-fancy-cats/classify
result: ❌

The error, as expected, was that getServerSideProps was trying to pass a value of .workflowID = undefined. This reflects the earlier comment I made.

Server Error
Error: Error serializing `.workflowID` returned from `getServerSideProps` in "/projects/[owner]/[project]/classify".
Reason: `undefined` cannot be serialized as JSON. Please use `null` or omit this value all together.

Status

Changes reque- wait,

UPDATE: while I was writing this review, @eatyourgreens replied to my earlier comment with the following:

If there’s no project, I want the workflow ID to be undefined. At least, I only want you to be able to classify if there’s a project and workflow.

Ah, in that case you need to figure out how to handle the issue when you run npm run dev and try to classify on I Fancy Cats. Additional details shortly.

packages/app-project/src/screens/ClassifyPage/getServerSideProps.js

shaunanoordin · 2021-01-06T18:02:54Z

Follow up details

If we have the else block, then getServerSideProps() has the potential to return props that looks like...

{
  host: 'http://localhost:3000',
  initialState: {
    project: {
      ...bla bla bla...
    },
    ui: { dismissedProjectAnnouncementBanner: null, mode: 'light' }
  },
  isServer: true,
  query: { owner: 'brooke', project: 'i-fancy-cats' },
  workflowID: undefined  // <-- this thing over here
}

No idea why it only occurs for yarn dev though.

EDIT: for clarification, this props works fine when returned by getServerSideProps()

{
  host: 'http://localhost:3000',
  initialState: {
    project: {
      ...bla bla bla...
    },
    ui: { dismissedProjectAnnouncementBanner: null, mode: 'light' }
  },
  isServer: true,
  query: { owner: 'brooke', project: 'i-fancy-cats' },
}

eatyourgreens · 2021-01-06T18:05:17Z

Weird. Maybe I Fancy Cats doesn’t have a default workflow set?

eatyourgreens · 2021-01-06T18:09:50Z

For the /classify route, it might be safer to check if the project only has one active workflow.

The logic here gets slightly hairy because one getServerSideProps function is being used across all the different /classify/* routes. Another approach would be to give each page its own getServerSideProps function.

EDIT: Checking project.links.active_workflows to see if there's only one workflow fixes I Fancy Cats, so I've gone with that.

Project app: move getInitialProps into a helper function, getDefaultPageProps, which fetches the project and sets initial UI state from cookies. Delete the app-level getInitialProps. Add getServerSideProps to each project app page (the home page, default classify page, workflow and subject set pages.)

undefined can't be serialised as JSON in the initial page props, so change the default to null in the UI store.

Restore the missing host, isServer and query props. isServer is always true for SSR.

workflowID and subjectSetID may not be present in the classify page URL, so assign them conditionally.

For projects with one active workflow, use that as the workflow ID on the Classify page.

Each /classify route has slightly different logic for generating page props. Handling them all in a single function leads to a bunch of if statements, which might cause bugs. Having a props function for each route seems to be cleaner. Add project.defaultWorkflow, which returns either the project's single active workflow or its default workflow.

eatyourgreens · 2021-01-07T10:11:27Z

Each page under /classify has slightly different logic for setting the workflowID prop. I've given each route its own getServerSideProps function, so that we don't have to deal with a bunch of if conditionals in a single function that tries to handle all possible routes.

That's going to have a knock-on effect for #1809 and #1875 but I think it's best to deal with that once this PR is finished.

eatyourgreens · 2021-01-07T11:29:57Z

While working on the tests, I found that the project model tests are broken, but still passing. I've opened #1960 to fix the tests.

shaunanoordin

PR Review

The latest iteration of this PR fixes the issues encountered by earlier tests.

Changes LGTM and tests pass fine 👍
I am noting that in the latest iteration, each page type (.../classify, .../classify/workflow/123, .../classify/workflow/123/subject-set/456) has its own version of getServerSideProps, which contrasts with the initial PR iteration which attempted to consolidate these versions into a single universal version.

Tests

Tests look good on...

Different commands: yarn build ; yarn start as well as yarn dev

Different projects:

http://localhost:3000/projects/brooke/i-fancy-cats/classify?env=staging - a project with one (active) workflow, and NO default workflow (Note: the one and only active workflow is chosen)
http://localhost:3000/projects/darkeshard/transformers/classify?env=staging - a project with multiple (active) workflows, and one default (active) workflow (Note: default workflow is correctly chosen.)

The only blorp I encountered was when you have a default workflow, but that workflow is inactive, but that's an edge case we don't need to worry about in this PR.

Status

This PR gets a green light for me. 👍

eatyourgreens added the enhancement New feature or request label Sep 24, 2020

eatyourgreens mentioned this pull request Sep 24, 2020

Render project workflows on the server #1809

Merged

18 tasks

eatyourgreens force-pushed the project-initial-props branch 3 times, most recently from 8afe635 to c3d1892 Compare September 29, 2020 11:32

eatyourgreens mentioned this pull request Oct 14, 2020

TypeError: Cannot read property 'active_workflows' of undefined #1694

Closed

eatyourgreens marked this pull request as draft October 22, 2020 10:27

eatyourgreens requested a review from a team October 22, 2020 10:27

eatyourgreens mentioned this pull request Nov 3, 2020

Projects: Add subject index pages #1875

Merged

18 tasks

eatyourgreens marked this pull request as ready for review December 10, 2020 16:51

eatyourgreens added the refactor Refactoring existing code label Dec 11, 2020

eatyourgreens assigned shaunanoordin Jan 5, 2021

eatyourgreens force-pushed the project-initial-props branch from 1dec9e9 to fc289f8 Compare January 5, 2021 13:33

shaunanoordin requested changes Jan 5, 2021

View reviewed changes

eatyourgreens force-pushed the project-initial-props branch from 7dfd43e to 3d64b7b Compare January 6, 2021 13:57

eatyourgreens force-pushed the project-initial-props branch from ad7bf1d to ed7e85e Compare January 6, 2021 15:56

shaunanoordin requested changes Jan 6, 2021

View reviewed changes

packages/app-project/src/screens/ClassifyPage/getServerSideProps.js Outdated Show resolved Hide resolved

eatyourgreens added 8 commits January 6, 2021 18:14

Default projectAnnouncementBanner to null

c9faa9e

undefined can't be serialised as JSON in the initial page props, so change the default to null in the UI store.

Tidy up pageProps

d7ea780

Restore the missing host, isServer and query props. isServer is always true for SSR.

Move getServerSideProps to src/screens

10baa89

Catch invalid properties in the project model

70b7ec5

Conditionally assign optional props

31f0c89

workflowID and subjectSetID may not be present in the classify page URL, so assign them conditionally.

Default workflow ID to the project default

128283d

Default behaviour for one active workflow

079dffd

For projects with one active workflow, use that as the workflow ID on the Classify page.

eatyourgreens force-pushed the project-initial-props branch from 1348c8f to 079dffd Compare January 6, 2021 18:24

Test the project default workflow

62fe95b

eatyourgreens mentioned this pull request Jan 7, 2021

Project app: Rewrite project model tests #1960

Merged

18 tasks

eatyourgreens mentioned this pull request Jan 7, 2021

Classifier: Project default workflow #1961

Merged

18 tasks

shaunanoordin approved these changes Jan 7, 2021

View reviewed changes

github-actions bot added the approved This PR is approved for merging label Jan 7, 2021

eatyourgreens merged commit 8292de4 into master Jan 7, 2021

eatyourgreens deleted the project-initial-props branch January 7, 2021 14:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Project app: use getServerSideProps on all project pages #1823

Project app: use getServerSideProps on all project pages #1823

eatyourgreens commented Sep 24, 2020 •

edited

Loading

eatyourgreens commented Oct 22, 2020

eatyourgreens commented Dec 10, 2020

shaunanoordin left a comment

eatyourgreens commented Jan 6, 2021

eatyourgreens commented Jan 6, 2021

eatyourgreens commented Jan 6, 2021 •

edited

Loading

shaunanoordin commented Jan 6, 2021

eatyourgreens commented Jan 6, 2021

shaunanoordin commented Jan 6, 2021

eatyourgreens commented Jan 6, 2021

shaunanoordin commented Jan 6, 2021

eatyourgreens commented Jan 6, 2021

shaunanoordin commented Jan 6, 2021

eatyourgreens commented Jan 6, 2021

shaunanoordin left a comment

shaunanoordin commented Jan 6, 2021 •

edited

Loading

eatyourgreens commented Jan 6, 2021

eatyourgreens commented Jan 6, 2021 •

edited

Loading

eatyourgreens commented Jan 7, 2021

eatyourgreens commented Jan 7, 2021

shaunanoordin left a comment

Project app: use getServerSideProps on all project pages #1823

Project app: use getServerSideProps on all project pages #1823

Conversation

eatyourgreens commented Sep 24, 2020 • edited Loading

Review Checklist

General

Components

Apps

Publishing

Post-merging

eatyourgreens commented Oct 22, 2020

eatyourgreens commented Dec 10, 2020

shaunanoordin left a comment

Choose a reason for hiding this comment

Approve

Dev Notes

Status

eatyourgreens commented Jan 6, 2021

eatyourgreens commented Jan 6, 2021

eatyourgreens commented Jan 6, 2021 • edited Loading

shaunanoordin commented Jan 6, 2021

eatyourgreens commented Jan 6, 2021

shaunanoordin commented Jan 6, 2021

eatyourgreens commented Jan 6, 2021

shaunanoordin commented Jan 6, 2021

eatyourgreens commented Jan 6, 2021

shaunanoordin commented Jan 6, 2021

eatyourgreens commented Jan 6, 2021

shaunanoordin left a comment

Choose a reason for hiding this comment

PR Review (Update)

Testing

Status

shaunanoordin commented Jan 6, 2021 • edited Loading

eatyourgreens commented Jan 6, 2021

eatyourgreens commented Jan 6, 2021 • edited Loading

eatyourgreens commented Jan 7, 2021

eatyourgreens commented Jan 7, 2021

shaunanoordin left a comment

Choose a reason for hiding this comment

PR Review

Tests

Status

eatyourgreens commented Sep 24, 2020 •

edited

Loading

eatyourgreens commented Jan 6, 2021 •

edited

Loading

shaunanoordin commented Jan 6, 2021 •

edited

Loading

eatyourgreens commented Jan 6, 2021 •

edited

Loading