holidayextras / jsonapi-server

A config driven NodeJS framework implementing json:api and GraphQL
MIT License
488 stars 115 forks source link

Hyphens no longer allowed but JSON API recommends them by default #206

Open gauthierm opened 8 years ago

gauthierm commented 8 years ago

In commit https://github.com/holidayextras/jsonapi-server/commit/b8a15576ac050e60385a2b4cdb4da0c1869268a9 it's no longer possible to define resources or attributes that contain hyphens.

The JSON API spec recommends using hyphens http://jsonapi.org/recommendations/#naming and other implementations expect this recommendation to be followed by default (for example, ember-data). It's possible to write a custom [de]serializer to make ember-data work with camelCase attributes but it would be nice to support the recommended defaults by default.

What's the rationale for removing hyphens as a valid character? Is it part of the new GraphQL support?

scottmtraver commented 8 years ago

I have this exact problem with a new ember app I am building! It was difficult to uncover because ember simply doesn't map them and the data returns but the models get undefined attributes

theninj4 commented 8 years ago

@gauthierm is correct, it's because GraphQL doesn't allow hyphens. I can understand their rationale - when writing JavaScript frontends (and backends), having to swap out the hyphens on both sides of the transport mechanism to dodge invalid syntax becomes a lot of hassle.

// this isn't valid JS:
person.email-address
// so we either have to chuck this everywhere:
person['email-address']
// or we mass-convert them to camelCase:
person.emailAddress
// or we simply drop the hyphens...

If you need the hyphens I'd recommend sticking to the ^1.17.0 release for now. I can look in to disabling the GraphQL support in favour of allowing hyphens?

theninj4 commented 8 years ago

For the record, when we jumped to v2.0.2 in production, we ditched all the hyphens and moved to camelCase.

adamlc commented 8 years ago

Whilst GraphQL is a nice to have, I don't think the library should break standards because of it. Maybe a happy medium would be allow hyphens and then transform them when they're used as part of GraphQL?

theninj4 commented 8 years ago

The naming convention with JSON:API is only a recommendation, it's not a part of the core spec: https://github.com/json-api/json-api/issues/323#issuecomment-74965368

We've gone with a "recommendation" for dasherized. This is not a part of the base
spec and of course won't be enforced. In #341, we originally made this a requirement
of the spec, but there was a lot of pushback and it was moved. Ultimately, names
and URLs are opaque to the spec.

http://jsonapi.org/recommendations/#naming

Here's a nice summary of what that means for Ember: http://stackoverflow.com/a/34546430 ...they're basically converting backwards and forwards.

I considered going down the path of rejigging all the hyphens into camelCase for GraphQL support. It was somewhat close to working, but alarmingly complicated (generating schema from other schema becomes quite the challenge). I began to question if this was a problem worth solving, and if we should simply drop the hyphens and make all our developers lives easier. Another way of approaching this - If we were to support hyphens for the REST routes and camelCase for GraphQL, then migrating from one to the other would be a lot of hassle, and would probably involve more on-the-fly conversions. Supporting two flavours of the same API, with different naming conventions, would also suck when offering a service to the wider world - imagine if GitHub's REST and GraphQL API's were to behave like that...

Right now I'm leaning towards letting users opt-out of the GraphQL support to enable them to use hyphens everywhere. I'd recommend anyone who is using Ember consider automating the conversion as per the top comment. I see conversions of camelCase to hyphens to camelCase as a waste of everyone's CPU, brain power, time and money - there are much better ways to be spending resource.

As a short-term fix, the ^1.17.0 release will behave as you want. The GraphQL release was an intentional major release to ^2.0.0, with (potentially) breaking changes.

adamlc commented 8 years ago

Sorry I didn't realise it was only a recommendation. TBH I didn't really like hyphens for the reason you mentioned earlier! For anyone wondering in Ember its actually pretty simple to convert to camelcase for example.

Its probably worth putting something in the readme about working with Ember. I guess a lot of people will probably be building APIs for ember.

I guess changing cases all the time would be a PITA and probably not worth the hassle. Going forward its probably OK to dump the hyphens and put something in the docs to help the Ember guys :)

theninj4 commented 8 years ago

You're right, the documentation should mention this - I'll get something added :+1:

gauthierm commented 8 years ago

Documentation for this issue (maybe something specifically for Ember) would be an acceptable fix for me. For now I'm using 1.17.0 but plan to update my ember project to use a camelCase serialize/deserialize. We are more than likely going to want GraphQL in the future so limiting ourselves now on a new project doesn't make sense.

mirovelk commented 7 years ago

I was hoping to use jsonapi-server as a frontend mock server for existing public json api. However it's using hyphens in resource names so I am unable to do so (because of the camel case restriction). Changing the api is not an option. Are there any plans to fix this? Updating documentation doesn't seem like a proper solution to me. Looks like this issue was kind of forgotten after adding documentation label...

EDIT: I do not need GraphQL.

Also when I add hyphen to the regexp that throws "contains illegal characters!" error it seems that everything works just fine (with "graphiql: false"). And with "graphiql: true" I get a different error ("Names must match"). So is there any reason not to allow hyphen in the first check?

SphtKr commented 7 years ago

Yes I think this may have been forgotten. I am--FWIW--fine with leaving the behavior as is, as long as there's a workaround and it's documented. The problem I'm hitting is that the guide link @adamlc referenced covers how to fix this for attribute names but not for resource names/URLs. I'd already done the former but I haven't yet figured out how to do the latter. That is, I have a resource with a two word name and jsonapi-server publishes it as /bigThings and Ember Data wants /big-things.