Resolvers change the content of date and time #835
Replies: 26 comments
-
@robross0606 Could you share a reproduction repo? |
Beta Was this translation helpful? Give feedback.
-
The spot where this happens looks pretty clear to me: export const parseTime = (time: string): Date => {
const currentDateString = new Date().toISOString();
return new Date(
currentDateString.substr(0, currentDateString.indexOf('T') + 1) + time,
);
}; Here you append the passed in time to the current date. Since Javascript date/time is relative to January 1st, 1970, a "free standing" time value should probably be relative to that date instead of the current date. Or at least there should be an option to do this: export const parseTime = (time: string): Date => {
const currentDateString = new Date().toISOString();
return new Date('1970-01-01T' + timeString + 'Z');
}; |
Beta Was this translation helpful? Give feedback.
-
However, personally I think converting these to a DateTime at all is a mistake. Validation of the value is one thing. Fundamentally altering the data type and, hence, the actual stored value and semantic meaning is another. Pushing an ISO Time value into a full Javascript DateTime object is a round peg in a square hole. |
Beta Was this translation helpful? Give feedback.
-
But it is not clear to me actually. :) |
Beta Was this translation helpful? Give feedback.
-
Okay, sorry, a Javascript |
Beta Was this translation helpful? Give feedback.
-
I've already decided that your library isn't meeting my needs. It is fundamentally altering data where I'm only looking to validate that the data conforms to ISO representation. This seems perfectly fine to you, so I'll be implementing my own custom scalars and not using the library at all. |
Beta Was this translation helpful? Give feedback.
-
The data is not changed actually, the time value is still there but it is a JS Date instance because there is no |
Beta Was this translation helpful? Give feedback.
-
What ends up being passed into the actual resolvers from input is fundamentally changed. Just because you convert it back and forth on the fringe, doesn't mean what is being handled internally based on the input is doing that. At the very least, your documentation is very much lacking in detail. I don't see anything that indicates these are even converted to Javascript GraphQL:
Resolver javascript code: module.exports = {
Mutation: {
testValidation: async (_, { input }) => {
// `input.time` contains the current date which is fundamentally wrong and we have to know to ignore it
const response = Arango.save(input) // Ooops we've just persisted incorrect data by saving a full date value the user never input
return safeStringify(input) // And here we spit back what was input purely so we can run a unit test.
}
}
} Because of this, the TimeResolver scalar isn't even idempotent. If I submit the same value tomorrow, it becomes a different value than it was today. |
Beta Was this translation helpful? Give feedback.
-
@ardatan I think two new scalars which simply validate the input and do not turn it into a Date object would be ideal. Eg Trying to use the While I understand interactions with |
Beta Was this translation helpful? Give feedback.
-
@marklawlor Why don't you think another scalar |
Beta Was this translation helpful? Give feedback.
-
@ardatan Most GraphQL clients serialise objects with JSON.stringify. Javascript's Date object has an unusual quirk where |
Beta Was this translation helpful? Give feedback.
-
@marklawlor I know but |
Beta Was this translation helpful? Give feedback.
-
Ah yes, forgot that toISOString converts to UTC. Seems reasonable to include DateTimeString 👍 |
Beta Was this translation helpful? Give feedback.
-
👋 I am also keen on adding a Time string to this library but my requirement is slightly different. My issue with the current time implementation is that forces a timezone either 'Z' (UTC) or the offset. What I am capturing in my app is the 24-hour time string without an offset, as the user is entering it irrespective of timezone. I imaging it would just validate "HH:MM:SS". That the hour range is 1 - 23, minute and second range 1-59 Would you be open to a 24TimeString (or some similar name)? I feel like TimeString is probably better reserved for the ISO version, but this is a unique and valid use case for a scalar. |
Beta Was this translation helpful? Give feedback.
-
@vespertilian - to solve a similar "dates and times without timezones" issue, my team introduced The fact that they aren't deserialized into a JS I'd be happy to submit a PR to this project with those scalars if it sounds like they'd solve your issue. |
Beta Was this translation helpful? Give feedback.
-
@chrskrchr Hey thanks. Looks kinda like what I want. I have also already created an implementation. I think I like your name better. Happy for either of us to do a PR. As long as others are happy with the content and naming. My implementation for reference: import { GraphQLError, GraphQLScalarType, Kind } from "graphql";
const UL_TIME = /(0[0-9]|1[0-9]|2[0-3]):[0-5][0-9]:[0-5][0-9]$/
export const GraphQLULTime = new GraphQLScalarType({
name: 'ULTime',
description: `
Unqualified local time also known as 24 hour time and military time.
Format: HH:mm:ss with no offset or timezone information.
Start at 00:00:00 and has a maximum values of 23:59:59
`,
serialize(value: unknown) {
if (typeof value !== 'string') {
throw new TypeError(`Value is not string: ${value}`);
}
if (!UL_TIME.test(value)) {
throw new TypeError(`Value is not valid unqualified local time (24 hour time): ${value} should be formatted HH:mm:ss`)
}
return value
},
parseValue(value: unknown) {
if (typeof value !== 'string') {
throw new TypeError(`Value is not string: ${value}`);
}
if (!UL_TIME.test(value)) {
throw new TypeError(`Value is not valid unqualified local time (24 hour time): ${value} should be formatted HH:mm:ss`)
}
return value
},
parseLiteral(ast) {
if (ast.kind !== Kind.STRING) {
throw new GraphQLError(
`Can only validate strings as ULTime but got a: ${ast.kind}`,
);
}
if (!UL_TIME.test(ast.value)) {
throw new TypeError(`Value is not valid unqualified local time (24 hour time): ${ast.value} should be formatted HH:mm:ss`)
}
return ast.value
}
}) Do these specs line up with what you were thinking?
|
Beta Was this translation helpful? Give feedback.
-
I like the name LocalTime :) @vespertilian Bit of a nitpick, but be aware the often military time does not include the colon and can include timezone in western countries. For example, 6:00 a.m. in zone UTC−5 is written "0600R" and spoken "oh six hundred Romeo" |
Beta Was this translation helpful? Give feedback.
-
@marklawlor, yeah sure I can remove the military time reference, I only added it when reading Wikipedia and looking for other names. If it's not accurate, lets remove it. Thanks. |
Beta Was this translation helpful? Give feedback.
-
@vespertilian - hope I'm not stepping on your toes, but I went ahead and submitted a PR of our solution that also includes support for a Our implementations of https://en.wikipedia.org/wiki/ISO_8601#Times So, I don't think this is necessarily a crazy thing to do. 😄 Let me know what you all think. |
Beta Was this translation helpful? Give feedback.
-
@chrskrchr, hey yeah submit a PR. No issues here :). I don't think we should have the default for a TS / JS library include the 24th hour, maybe this scalar could be a function you import and you pass a parameter Or the scalar could accept 24:00:00 and rewrite it to 00:00:00? That way whenever it goes in or comes out there would be no errors in frontend/backend JS TS libraries. |
Beta Was this translation helpful? Give feedback.
-
PR submitted here: #533
That's a fair point. Both Moment and Luxon do currently support Rather than making it a function-scalar, I'd rather introduce a separate scalar called |
Beta Was this translation helpful? Give feedback.
-
@chrskrchr local end time sounds good. |
Beta Was this translation helpful? Give feedback.
-
Great - I've split the time scalar into |
Beta Was this translation helpful? Give feedback.
-
We never actually created the DateTimeISOString did we? I think I want that now. I want the date validated against the ISO spec but I don't want it transformed into a Date object as I will just have to transform it back to write it to the DB. @ardatan Are you good for a PR with this? |
Beta Was this translation helpful? Give feedback.
-
Correct. While the |
Beta Was this translation helpful? Give feedback.
-
I moved this issue to Discussions. |
Beta Was this translation helpful? Give feedback.
-
When using
TimeResolver
orDateResolver
, the actual values are modified. For example, if I put in a time value of "00:00:30Z", what is actually passed through to mutation resolver code for storage and response is a full DateTime which includes the current date.This is not functionally accurate and changes the fundamental semantic meaning of the values.
Beta Was this translation helpful? Give feedback.
All reactions