Parse ObjectID scalar to Mongo ObjectID #429
Comments
|
@machi1990 I already tried to return |
Hi @ardatan thanks for the reply. Are there plans to have such a support e.g by investigating the treeshaking issue further? |
|
@machi1990 yes we would be open for this change, would you care to try a PR that might tackle this issue? |
|
Webpack assumes exported |
|
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 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 |
|
@ash0080 There are multiple subjects here :) First, ObjectID should always string in our scalar (if not this is a bug). |
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 |
|
My concern about
|
|
You can just extract the code from ObjectID or let the user pass in an instance |
|
What do you mean by extracting code from ObjectID and how do we decide if that is an instance of ObjectID without importing |
see? Why do you want to reference the entire mongodb? |
|
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. |
|
It doesn't make sense, In the end, you can specify mongodb as an external module in webPack ::jack_o_lantern: |
|
@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. |
|
Dynamic Import? Dependency Injection? Factory pattern? many ways to solve your problem if you wish |
|
@ash0080 Personally I couldn't make it fixed in anyways. If you think you can fix it, PRs are always welcome. |
|
Not interested in doing any fix, Because I think this module is badly designed, I plan to rewrite a new |
|
@ash0080 Thank you anyway :) and good luck on your new library 🤞 . I am looking forward to seeing it |
|
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? |
|
I don't have any ideas, I don't even intend to discuss how to make a bundle with webpack, But maybe, you guys would like to see how graphql-validated-types is designed, 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 |
|
@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. |
The `ObjectID` scalar type does some great work ensuring that the data
in the database maps to valid ObjectID values for MongoDB. However, in
order to use arguments supplied as ObjectID, we still need to convert
them to a `bson.ObjectId` type on-the-fly in resolvers or else we'll end
up not matching documents that should be matched.
This change adds a dependency on the `bson` library so we can use its
`ObjectId` class as a means of serializing ObjectID data from the
GraphQL API into a usable object for resolvers.
Example of code before this change:
```javascript
someResolver: (_parent, { id }, { db }) => {
const someData = await db.collection('things').findOne({ _id: new ObjectId(id) })
},
```
And here's what it will look like afterward:
```javascript
someResolver: (_parent, { id }, { db }) => {
const someData = await db.collection('things').findOne({ _id: id })
},
```
Similar to `Date` objects which are serialized appopriately in MongoDB
if you use `Timestamp`, ObjectIDs should be parsed properly into their
correct type before persistence into the database. By doing so, we're
ensuring that this type is always consistent.
Resolves Urigo#429
|
@ardatan Not sure if this was always the case, but the I have a demonstration of this in tubbo@861d9db if you want to see what I was thinking. |
The `ObjectID` scalar type does some great work ensuring that the data
in the database maps to valid ObjectID values for MongoDB. However, in
order to use arguments supplied as ObjectID, we still need to convert
them to a `bson.ObjectId` type on-the-fly in resolvers or else we'll end
up not matching documents that should be matched.
This change adds a dependency on the `bson` library so we can use its
`ObjectId` class as a means of serializing ObjectID data from the
GraphQL API into a usable object for resolvers.
Example of code before this change:
```javascript
someResolver: (_parent, { id }, { db }) => {
const someData = await db.collection('things').findOne({ _id: new ObjectId(id) })
return someData
},
```
And here's what it will look like afterward:
```javascript
someResolver: (_parent, { id }, { db }) => {
const someData = await db.collection('things').findOne({ _id: id })
return someData
},
```
Similar to `Date` objects which are serialized appopriately in MongoDB
if you use `Timestamp`, ObjectIDs should be parsed properly into their
correct type before persistence into the database. By doing so, we're
ensuring that this type is always consistent.
Resolves Urigo#429
The `ObjectID` scalar type does some great work ensuring that the data
in the database maps to valid ObjectID values for MongoDB. However, in
order to use arguments supplied as ObjectID, we still need to convert
them to a `bson.ObjectId` type on-the-fly in resolvers or else we'll end
up not matching documents that should be matched.
This change adds a dependency on the `bson` library so we can use its
`ObjectId` class as a means of serializing ObjectID data from the
GraphQL API into a usable object for resolvers.
Example of code before this change:
```javascript
someResolver: (_parent, { id }, { db }) => {
const someData = await db.collection('things').findOne({ _id: new ObjectId(id) })
return someData
},
```
And here's what it will look like afterward:
```javascript
someResolver: (_parent, { id }, { db }) => {
const someData = await db.collection('things').findOne({ _id: id })
return someData
},
```
Similar to `Date` objects which are serialized appopriately in MongoDB
if you use `Timestamp`, ObjectIDs should be parsed properly into their
correct type before persistence into the database. By doing so, we're
ensuring that this type is always consistent.
Resolves Urigo#429
The `ObjectID` scalar type does some great work ensuring that the data
in the database maps to valid ObjectID values for MongoDB. However, in
order to use arguments supplied as ObjectID, we still need to convert
them to a `bson.ObjectId` type on-the-fly in resolvers or else we'll end
up not matching documents that should be matched.
This change adds a dependency on the `bson` library so we can use its
`ObjectId` class as a means of serializing ObjectID data from the
GraphQL API into a usable object for resolvers.
Example of code before this change:
```javascript
someResolver: (_parent, { id }, { db }) => {
const someData = await db.collection('things').findOne({ _id: new ObjectId(id) })
return someData
},
```
And here's what it will look like afterward:
```javascript
someResolver: (_parent, { id }, { db }) => {
const someData = await db.collection('things').findOne({ _id: id })
return someData
},
```
Similar to `Date` objects which are serialized appopriately in MongoDB
if you use `Timestamp`, ObjectIDs should be parsed properly into their
correct type before persistence into the database. By doing so, we're
ensuring that this type is always consistent.
Resolves Urigo#429
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
ObjectIDand 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
The text was updated successfully, but these errors were encountered: