Parse ObjectID scalar to Mongo ObjectID

[machi1990]

Currently the ObjectID scalar only checks for conformity with the Mongo ObjectID

We, at Graphback are considering the usage of this scalar type, we are considering the options of parsing the value directly to ObjectID and not return the raw string. In doing so, thought that this is a good feature request in upstream.

Opening for consideration and discussion.

/cc @craicoverflow @Urigo @wrocki

[ardatan]

@machi1990 I already tried to return ObjectID instead of string but treeshaking didn't work well with mongodb package. That's why, I kept ObjectID scalar as it is.

[machi1990]

@machi1990 I already tried to return ObjectID instead of string but treeshaking didn't work well with mongodb package. That's why, I kept ObjectID scalar as it is.

Hi @ardatan thanks for the reply. Are there plans to have such a support e.g by investigating the treeshaking issue further?

[Urigo]

@machi1990 yes we would be open for this change, would you care to try a PR that might tackle this issue?

[machi1990]

@Urigo @ardatan I gave it a try here but the bundle size more than doubled.

./dist/index.js: 191.88KB > maxSize 85KB (no compression) 

Was this the approach that you considered? @ardatan

[ardatan]

Webpack assumes exported GraphQLScalarType instances have always side effects. For most of them, pure magic comment works; https://github.com/Urigo/graphql-scalars/blob/master/src/scalars/ObjectID.ts#L5 But it doesn't work well with MongoDB's ObjectID somehow.

[ash0080]

ObjectID should return a fixed type, but now, it could be a string or an ObjectID, A basic principle should be at least that the incoming and outgoing are the same type.

Same case, e.g. URL, incoming is a string, return type is an URL object, The silliest thing is that it's stored in the database and my storage has grown 110% as a result. And really, all one needs is a regex check besides, PhoneNumber can't work without a '+' (???) ...

Sorry for gushing, but please understand that I struggled for hours and eventually found I had to rewrite resolvers myself, There are so many incredible

[ardatan]

@ash0080 There are multiple subjects here :) First, ObjectID should always string in our scalar (if not this is a bug). Second, you can easily stringify incoming URL object with 'toString'. The scalar uses URL object like DateTime scalar uses Date object which can be easily formatted with 'toJSON' or 'toString' easily. Those are already mentioned in the docs afaik. If you want to have a validation only and keep the incoming string as-is, you can use RegularExpression factory to create your scalars easily.

[ash0080]

const { GraphQLScalarType, Kind, GraphQLError } = require('graphql')
const MONGODB_OBJECTID_REGEX                    = new RegExp('^[0-9a-fA-F]{24}$')
const URL_REGEX                                 = new RegExp('^(?:http(s)?:\\/\\/)?[\\w.-]+(?:\\.[\\w\\.-]+)+[\\w\\-\\._~:/?#[\\]@!\\$&\'\\(\\)\\*\\+,;=.]+$')
const { ObjectID }                              = require('mongodb')
const validateObjectId = value => {
  if (typeof value !== 'string' || !(value.length === 24 && MONGODB_OBJECTID_REGEX.test(value))) {
    throw new TypeError(`Value is not a valid mongodb object id of form: ${value}`)
  }
  return ObjectID(value)
}
const validateURL      = value => {
  if (!URL_REGEX.test(typeof value === 'string' ? value.trim() : value)) {
    throw new TypeError(`Value is not a valid URL of form: ${value}`)
  }
  return value
}
module.exports         = {
  ObjectID: new GraphQLScalarType({
                                    name: 'ObjectID',
                                    description: 'MongoDB ObjectID type.',
                                    parseValue (value) { // value from the client
                                      return validateObjectId(value)
                                    },
                                    serialize (value) { // value sent to the client
                                      if (ObjectID.isValid(value)) {
                                        return value.toHexString()
                                      }
                                      throw new TypeError(`Value is not a valid ObjectID of form: ${value}`)
                                    },
                                    parseLiteral (ast) { // ast value is always in string format
                                      if (ast.kind !== Kind.STRING) {
                                        throw new GraphQLError(`Can only validate strings as mongodb object id but got a: ${ast.kind}`)
                                      }
                                      return validateObjectId(ast.value)
                                    }
                                  }),
  URL: new GraphQLScalarType({
                               name: 'ObjectID',
                               description: 'A field whose value conforms with the standard mongodb object ID as described here: https://docs.mongodb.com/manual/reference/method/ObjectId/#ObjectId. Example: 5e5677d71bdc2ae76344968c',
                               serialize: validateURL,
                               parseValue: validateURL,
                               parseLiteral (ast) {
                                 if (ast.kind !== Kind.STRING) {
                                   throw new GraphQLError(`Can only validate strings as mongodb object id but got a: ${ast.kind}`)
                                 }
                                 return validateURL(ast.value)
                               }
                             })

I've given up on this. rewrite is faster, besides, ObjectID support auto conversion now

update, fix [ObjectID] support

[ardatan]

My concern about ObjectID is the bundle size. Treeshaking doesn't work well with mongodb package so it increases bundlesize heavily and I don't want to use ObjectID from mongodb directly. For URL, you can use RegularExpression factory like below;

const MyRegexType = new RegularExpression('MyRegexType', /^ABC$/);

[ash0080]

You can just extract the code from ObjectID or let the user pass in an instance

[ardatan]

What do you mean by extracting code from ObjectID and how do we decide if that is an instance of ObjectID without importing mongodb?

[ash0080]

node_modules/bson/lib/bson/objectid.js 

see? Why do you want to reference the entire mongodb? You can use bson-objectid, it's 33k,Even if you quote the entire bson, it's only 2.5M

[ardatan]

The entire GraphQL Scalars package is 90 kB and even 33 kB is too much compared to entire size but all scalars are treeshakable so for example if you use only one scalar, you will only have that scalar's bundle size. If we use any other package for ObjectID, that might not work with MongoDB driver because it uses bson npm library and sometimes it uses bson-ext if available.

[ash0080]

It doesn't make sense, In the end, you can specify mongodb as an external module in webPack ::jack_o_lantern:

[ardatan]

@ash0080 Having 'mongodb' as an external dependency doesn't matter. How do you manage non Node environment? Since there is no treeshaking for ObjectID, mongodb will be needed eventually even if you don't use ObjectID scalar.

[ash0080]

Dynamic Import? Dependency Injection? Factory pattern? many ways to solve your problem if you wish

[ardatan]

@ash0080 Personally I couldn't make it fixed in anyways. If you think you can fix it, PRs are always welcome.

[ash0080]

Not interested in doing any fix, Because I think this module is badly designed, I plan to rewrite a new

[ardatan]

@ash0080 Thank you anyway :) and good luck on your new library 🤞 . I am looking forward to seeing it

[Urigo]

Jumping in here, @ash0080 I think it's hard to read your thoughts, maybe instead of the debate here, you could open a draft PR on how you think it should look like, maybe the discussion would be more clear there?

[ash0080]

I don't have any ideas, I don't even intend to discuss how to make a bundle with webpack, and I don't care if the package is intended to support ObjectID conversions. Because I have removed it from my package.json

But maybe, you guys would like to see how graphql-validated-types is designed,
Although it has only 1/800 of your downloads.

if it were me, I would like to base a module on the factory pattern instead of a bunch of inflexible static declarations, even though the current package is 90k, it contains many types that are not actually usable.

That's why I think it's pointless to discuss whether to open up some configuration here

[ardatan]

@ash0080 There are many people using this package in their serverless or client side projects so they need to think about bundle size. That's why, we care about the bundle size. This project is open source and we always accept PRs from the community to make this package better for the community. I am sorry to hear you don't want to contribute this project. It is hard to follow your thoughts in this conversation, that's why @Urigo asked a compiled version of all of your ideas which are really important to us. Like which ones do you think unusable, how would you do that etc? But if you already found what you're looking for, that's good :)

[tubbo]

@ardatan Not sure if this was always the case, but the mongodb package imports its ObjectID type from bson. This would theoretically allow a library such as this one to import that object without needing to import the entire mongodb library, which I would also agree is not feasible for front-end projects using this package.

I have a demonstration of this in https://github.com/tubbo/graphql-scalars/commit/861d9db1f8ed4115090f206503f9c31b0105b2c2 if you want to see what I was thinking.