Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Expiration Date" feature for Community Hero and Events #1281

Merged
merged 58 commits into from
May 15, 2020

Conversation

rogermparent
Copy link
Contributor

@rogermparent rogermparent commented May 11, 2020

The hero and events in the community page's data.json now accept an
optional expires field, which stops the item in question from rendering when
the site is built past that date.

This is done through a new shared expiration helper and its child function
isExpired, which uses the moment library that Gatsby already depends on to
determine if an arbitrary content object is expired.

Any object with a date and/or expires key that can be parsed by moment is
usable in isExpired. Objects without an expires will be treated as if they
have an expires 7 days after their date; this is configurable in a constant.

Also, the expiration helper is in ES5 so it can be used on both during the build
and at runtime.

Made for and fixes #1277
Currently includes #1282's files, but not its commits.

Gatsby already depends on moment, this just lets us use it in our code. This
branch uses it to work with expiry dates.

The yarn upgrade just affects yarn.lock, any pinned packages (like the image
ones we need for caching to work) are left alone.
The hero and events in `community` now accept an optional `expires` field, which
stops the item in question from rendering when the site is built past that date.

This is done through a new shared expiration helper and its child function
`isExpired`, which uses the `moment` library that Gatsby already depends on to
determine if an arbitrary content object is expired.

Any object with a `date` and/or `expires` key that can be parsed by `moment` is
usable in `isExpired`. Objects without an `expires` will be treated as if they
have an `expires` 7 days after their `date`; this is configurable in a constant.

Also, the expiration helper is in ES5 so it can be used on both during the build
and at runtime.
@rogermparent rogermparent marked this pull request as draft May 11, 2020 19:00
@shcheklein shcheklein temporarily deployed to dvc-landing-invalidate--narsvs May 11, 2020 19:00 Inactive
@rogermparent
Copy link
Contributor Author

The basic functionality is working, but I need to fix lint issues and make sure edge cases are handled correctly. Others can also check it out to give their opinion, but know it's currently not quite final.

elleobrien and others added 10 commits May 11, 2020 13:36
These enable data previously imported from JSON files to run through
the Gatsby GraphQL interface, allowing us to do arbitrary transformations
and queries of this data.

Building on top of these models allows for more complex features to be
built either on top or in place of the previously imported JSON files.

This is used in expiration dates to do all date parsing and comparisons
at build time, allowing us to query for non-expired events in whatever order
we want (including the original source order) in either direction.

The JsonFiles model differs from `gatsby-source-json` by taking the approach of
simply exposing parsed JSON content to other models to consume instead of
inferring a node schema. It's particularly useful for our large JSON files, but
also simpler to adapt to the possibility of us breaking those files up or even
getting the data from another source. It actually came from one of my personal
branches where I make the sidebar run on this logic, but that ended up being
unnecessary without a feature to justify the change. It will likely be reused in
the sidebar overhaul.

The Community model will handle parsing the community JSON data and reading it
into GraphQL. This becomes much easier by moving the `data.json` from the
`Community` component into the `content` folder. I know we value co-location,
but co-locating this file would mean pointing `gatsby-source-filesystem` at our
entire source folder. It makes a lot of sense for this to be in `content` as
well.

This also changes the expiration helpers, splitting the old
`isExpired({date,expires})` into `getExpirationDate({date,expires})` and
`isExpired(expires)`. This allows consumers to get the exact expiration date as
well as be able to use a boolean.

The new expiration helpers also consider `expires=false` to mean the given
object never expires. These objects will return `false` when given to
`getExpirationDate` and `isExpired` will immediately return `false` when given
`false`. Since the result of both helpers are added to the GraphQL node, these
implementation details may never come up.
This allows for better build-time processing, enabling things like
build-time querying for non-expired Events and Heroes.

Instead of importing the JSON file directly, Community content data is now
accessed through a custom React hook that wraps `useStaticQuery` and massages
the data into a more usable form.

Events and Heroes are given custom node types, and everything else in the
community json file is carried over in a unique "rest" node that exposes the
rest of the properties in JSON.

The Hero and Event components on the community page are adapted to the new
custom nodes, and the other Community components are successfully working
through the Rest node- they can be truly ported later or left as-is if they
don't need to change.
- url changed to `#subscribe` fragment in `data.json`
- hard-coded `target="_blank"` removed from component
- `scroll-behavior: smooth` added to Page base CSS so Fragment links
  smooth-scroll.
This also includes the fixes to `target`. This should be handled in the `Link`
wrapper, so we don't have to hard-code it in the Hero component.
@rogermparent rogermparent temporarily deployed to dvc-landing-invalidate--narsvs May 12, 2020 20:59 Inactive
@rogermparent
Copy link
Contributor Author

rogermparent commented May 12, 2020

Just pushed some big changes, moving Community data into GraphQL so we can transform it at build time (like filtering out expired Events/Heroes) with no runtime or bundle cost. I still need to fix lint issues, but local builds work.

I added in the header from #1282 to show off the "multiple expiring heroes with a fallback" feature. I only wanted the files so I used checkout to get them without the commits. I don't know if I'll have to reconcile that if that PR gets merged first (and it likely will), but I'm sure it won't be too difficult.

elleobrien and others added 6 commits May 12, 2020 14:05
This outfits the `useCommunityData` hook with TS definitions which allow it to
pass `yarn ts-lint`. I'm sure they'll have to be changed in the future, but now
that we have them refactoring will be less of an issue.
@rogermparent rogermparent temporarily deployed to dvc-landing-invalidate--narsvs May 12, 2020 22:44 Inactive
@rogermparent
Copy link
Contributor Author

yeah, the problem with not rendering it is that we still have in the Menu (and potentially other outside links we don't have control over). So, it's better to render it with some message "No upcoming events. Subscribe to be up to date point_down"

Good point! I'll go with the text then.

@rogermparent rogermparent had a problem deploying to dvc-landing-invalidate--narsvs May 14, 2020 03:34 Failure
@rogermparent
Copy link
Contributor Author

I canceled the deploy yesterday because it started to take forever with odd errors, and I wanted to get a good night's rest before I go in on Heroku issues.

Thankfully, it seems the only issue is that the caches were incompatible and a full rebuild succeeded- though it took 800s, the cache is fresh now and the next build should be normal. I'll report back to confirm this when I do it. I'll try a quick test now and then keep an eye on it when I make unit tests for the expiration behavior.

@rogermparent rogermparent temporarily deployed to dvc-landing-invalidate--narsvs May 14, 2020 22:12 Inactive
@rogermparent
Copy link
Contributor Author

rogermparent commented May 14, 2020

There was an odd issue where a field not being explicitly declared broke cache re-use on Heroku alone, but it seems like adding it did the trick! I didn't even have to revert the yarn upgrade.

On that note, the newer version of Gatsby the branch logs which specific queries are taking too long!

warning Query takes too long:
File path: /tmp/build_c87156d0007824c1d78716f29e5d33db/src/templates/blog-home.tsx
URL path: /blog
Context: {
    "isBlog": true,
    "limit": 7,
    "pageInfo": {
        "currentPage": 1,
        "nextPage": "/blog/page/2"
    },
    "skip": 0
}

This will be a valuable clue on how to improve build time further, as well as keep it low in the face of future additions!

I couldn't quickly find any existing class to compose over, so I made one in the
main Community module.
@rogermparent rogermparent temporarily deployed to dvc-landing-invalidate--narsvs May 14, 2020 23:33 Inactive
@rogermparent rogermparent temporarily deployed to dvc-landing-invalidate--narsvs May 15, 2020 02:03 Inactive
@rogermparent rogermparent requested a review from shcheklein May 15, 2020 02:10
@shcheklein
Copy link
Member

Not merging at night, let's do this tomorrow - since we had some problems with cache or something (will need to invalidate it, right?)

@shcheklein
Copy link
Member

Btw, the "upcoming events text" is still black for me - is it expected?

@rogermparent
Copy link
Contributor Author

@shcheklein I'm not sure why it's still black, it was gray in development for me. I'll fix that quickly, it's supposed to be gray.

I don't think we'll have to invalidate cache, the only reason I had to before was because an inferred field behaved differently on Heroku, breaking the first build and triggering the fallback "clean and retry" mechanism. Now that the field is explicitly defined, the caches should be compatible enough not to re-trigger image generation.

@rogermparent rogermparent temporarily deployed to dvc-landing-invalidate--narsvs May 15, 2020 12:42 Inactive
@rogermparent
Copy link
Contributor Author

rogermparent commented May 15, 2020

Btw, the "upcoming events text" is still black for me - is it expected?

I had a commit that fixed a typo in the class that made it gray for me- turns out our verify step rejects commits that contain only case changes because it sees them as unchanged, and I missed that before I stopped last night.
It went through fine with no-verify, but that's a quirk worth remembering.

EDIT: Oh, it's a CSS lint thing. I suppose I can change the class I used to a name that sidesteps this issue.

This avoids a css lint issue, and arguably makes more sense.
@rogermparent rogermparent temporarily deployed to dvc-landing-invalidate--narsvs May 15, 2020 13:57 Inactive
@rogermparent rogermparent temporarily deployed to dvc-landing-invalidate--narsvs May 15, 2020 16:16 Inactive
@shcheklein shcheklein merged commit 5e5a993 into master May 15, 2020
@rogermparent rogermparent mentioned this pull request May 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

community page: events expiration date, do not show irrelevant events
3 participants