Skip to content
This repository has been archived by the owner on Feb 1, 2024. It is now read-only.

feat(isostring): add ISOString Custom Scalar to apollo-utils #220

Merged
merged 10 commits into from
Feb 15, 2023

Conversation

cmharlow
Copy link
Contributor

@cmharlow cmharlow commented Feb 14, 2023

Goal

Add a new component to this package - the custom graphql scalar ISOString (which takes in null or a ISO-8601 compliant, UTC string for input, expects a Typescript Date object or null on the server side).

Reference

Documentation here:

  • See ticket for links to docs & PocketSave work that prompted using this custom scalar
  • Slack convo then led to wanting this custom scalar to be in apollo-utils for reuse across various subgraphs

Issues:

Implementation Decisions

We don't handle 0000-00-00 type dates, which we cheat with in our Aurora DB to indicate a None type. Instead, this Scalar expects your db client & data layer logic converts those MySQL 0000-00-00 dates to null. And if you want to query on these, you pass through GraphQL a null.

The expectation also for dates going into GraphQL from clients are ISO8601 compliant following the strictest expectations & requiring you are passing in an explicit UTC timezone. but not the strictest interpretation (e.g. MySQL style ISO 8601 dates are okay too)

We also always expect dates going into GraphQL from clients are UTC. If clients paste in SQL style dates, these are forced to UTC. ISO compliant dates passed in to GraphQL queries etc. with offsets will be honored, just not advertised. if other timezones/offsets are passed in, they are rejected.

We always expect that for dates being pulled from a database or data store, the database client & data layer logic handles the timezone required for the db session before handing a TS Date object off to the Server. For the server, it only knows a Typescript Date object. For passing that object to GraphQL outputs (to hand back to clients), we convert these Dates using toISOString, which only outputs UTC (according to these docs, but I'm happy to be corrected here).

@cmharlow cmharlow requested a review from a team as a code owner February 14, 2023 22:08
Copy link
Contributor

@hyperparabolic hyperparabolic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to take another pass at this to go over tests tomorrow morning, but I wanted to get the code style feedback you asked for here and ready for you now.

README.md Outdated Show resolved Hide resolved
src/isoStringScalar/isoStringScalar.ts Show resolved Hide resolved
src/isoStringScalar/isoStringScalar.ts Outdated Show resolved Hide resolved
src/isoStringScalar/isoStringScalar.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@hyperparabolic hyperparabolic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test coverage looks great!

I think the only other thing missing here is that we need to export the scalar in the top level index so that this can be consumed externally. If we want to plan for future scalars and make adoption of these a bit more automatic in the future, It would be nice to export this as an object that can be spread into the consuming server's resolvers in addition to individual exports.

exporting:

export const PocketDefaultScalars = {
  ISOString: isoStringScalar,
};

importing:

const resolvers = {
  ...PocketDefaultScalars,
  // ...other resolvers...
  // if another server needs a different internal representation
  // they can still override the defaults below:
  // ISOString: someNonDefaultScalar
};

Renovate updates alone will enable the usage of new scalars without devs having to modify imports / resolvers.

src/isoStringScalar/isoStringScalar.ts Show resolved Hide resolved
src/isoStringScalar/isoStringScalar.ts Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/isoStringScalar/isoStringScalar.ts Show resolved Hide resolved
cmharlow and others added 4 commits February 15, 2023 13:02
Co-authored-by: Kat Schelonka <34227334+kschelonka@users.noreply.github.com>
Co-authored-by: Kat Schelonka <34227334+kschelonka@users.noreply.github.com>
@cmharlow
Copy link
Contributor Author

cmharlow commented Feb 15, 2023

...PocketDefaultScalars,
// ...other resolvers...
// if another server needs a different internal representation
// they can still override the defaults below:
// ISOString: someNonDefaultScalar

dang thanks for catching that I didn't export it at all @hyperparabolic !

The export object is a great idea. I tried to implement like you described.

Copy link
Contributor

@hyperparabolic hyperparabolic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@cmharlow cmharlow merged commit 6cf9bd2 into main Feb 15, 2023
@cmharlow cmharlow deleted the INFRA-812/isoscalar-impl branch February 15, 2023 22:45
@pocket-ci
Copy link
Collaborator

🎉 This PR is included in version 3.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants