graphiti-api / spraypaint.js

Graphiti Client / Javascript ORM / JSONAPI
MIT License
108 stars 69 forks source link

.save doesn't serialize attributes? : (RESOLVED - TypeScript compatibility issue) #111

Closed corrspt closed 2 years ago

corrspt commented 2 years ago

Hey everyone,

(see solution at end of post, or read the entire thread)

I'm starting a new project in VueJS, coming from an Ember project, and both will be using the same JSON:API backend. I looked around for libraries to Help with the JSON:API in the Vue project and landed on Spraypaint after a bit of search, and it seems a perfect match, but I'm having an issue that I can't simple figure out.

This is my base model:

import { Model, SpraypaintBase } from 'spraypaint';
import envConfig from '@/configEnv.js';

@Model()
export class ApplicationRecord extends SpraypaintBase {
  static baseUrl = envConfig.url;
  static apiNamespace = '/api';
}

And this is my model

import { Model, Attr } from 'spraypaint';
import { ApplicationRecord } from './BaseAssemblyModel';

@Model()
export class Client extends ApplicationRecord {
  static jsonapiType = 'project-clients';

  @Attr() name: string | null;
}

I can query just fine, but I have an issue with saving (in this case, updating an existing model, but the same happens when creating a new one)

When I ask to await client.save() the json that is sent to the backend doesn't include the name property defined in the model. If I inspect the data however, it's there: image

However, the serialized JSON doesn't have it. And apparently, from what I can investigate... it's because of this: image The attributes property returns empty all the time.

Digging more, we find this: image

The typedAttributes always returns empty :(. Further digging, and It seems the model._attributes is never set, I've put debugger statements, live debugging, but don't find a place where _attributes is set and it's always empty.

It seems such a basic thing, that I'm not sure what I'm doing wrong... any ideas? Thanks :)


Solution: The compilerOptions of tsconfig.json needed to be changed, we had target: esnext and needed to change to target:es2017. Thanks @bilouw for the help

bilouw commented 2 years ago

You're clearly missing something ! Of course, attributes that has been changed (dirty) should be included in the payload sent to the back.

Can you please share your code, where you create then save your model to see if something is wrong ?

corrspt commented 2 years ago

Hey @bilouw

FYI: I'm using VueJS 2, with TypeScript 4.5 (I'm new to TypeScript/VueJs)

Thanks a bunch for the fast reply. I tried a simpler test to isolate things, and got two different things:

This one didn't work (same thing doesn't send the attribute) and this is closer to what I have:

let client = new Client();
client.name = 'Test 1';
await client.save();

This one worked and the record was saved and the attribute serialized to the server

let client = new Client({ name: 'Test 2' });
await client.save();

The model definition was the same as I shared before.

The actual situation... there's a list of Client instances and the user clicks on one an opens a modal, which receives the Client instance as a prop. And in the form I have this (CInput is a custom component but basically a form)

<v-col cols="12">
     <CInput v-model="client.name" />
 </v-col>

And on the form submit:

@Emit('onSave')
    save() {
      return this.client; <--- This has the correct value for name
    }

The place where the emit is handled:

async saveClient(client: Client) {
       await client.save(); <--- Client here is also correct with the name attribute... only when the serialization occurs it disappears)
      // More stuff not relevant
bilouw commented 2 years ago

hmmm, weird, test 1 and test 2 should both work ! You can instantiate client with his name, or set it after like in test 2, both should work. It's a very simple case, don't see what can go wrong.

Can you try to change the 'name' attribute type for only string (in model). This way, it will always be initialize with ''. I wonder if it could be a reactivity problem.

EDIT: Also, take a look at this: https://www.graphiti.dev/js/writes/dirty-tracking and try to see if your client attribute is dirty

corrspt commented 2 years ago

Hey @bilouw

Thanks for the quick reply. I've spent sometime around this on the weekend and today. Here's what I found with the help of a colleague. We started a vue new project and added SprayPaint.js and we tried :

Which is interesting because I then found this issue: https://github.com/graphiti-api/spraypaint.js/issues/32 Which talks about an issue with TypeScript 3.5 and above, but doesn't really say what happens when TS is above 3.5.

Not sure If I can conjure a PR to fix the TypeScript thing, as I'm a TypeScript newbie, but if I can... do you think a PR would be merged and released? (I'm looking at the list on unmerged PR's and I don't know if maintainers don't want to merge, or don't have the time to review them - Not judging, I understand the problem).

Again, thanks for giving my issue your attention :)

bilouw commented 2 years ago

You should be able to use spraypaint with typescript 4.5.5. At my work, we use it in a Vue project with TS 4.5.5 (and above)

I think your problem comes from the target compilation (in tsconfig.json). Personally, i use target "es2017" and module "esnext". Try to use these target compilation with TS 4.5.5 and let me know if it's work.

Do not confuse two things: The version of TS that you're using in your project, and the version of TS that has been used to write the spraypaint lib. Spraypaint need an update of his TS version, but you can still use it with TS 4.5.5.

We have a lot of PR to merge, and the lib could be improved every day, just a lack of time. I work every day with graphiti/spraypaint but don't have time these days to improve it ...

corrspt commented 2 years ago

Hi @bilouw

Thanks again for the help, it did work as you said.

As we're building the app alongside other apps that all share the same tsconfig.json I'll have to ask if there's anything that would prevent us from using the es2017 target. Hope not :)

Nice to know about that, I'm just getting into Typescript so the compatibily landscape is something that eludes me.

Is Spraypaint an open-source project that you maintain only by yourself (but use it at work?). I totally understand the constraints in evolving an open-source library, triaging issues like this, looking at PRs and much appreciate it being shared. As we're evaluating the library, if we use it we will probably be able to provide some assistance (even if only in documentation PR help?).

Does it make sense to provide a PR with this information? I'll update the title of the issue with more information

bilouw commented 2 years ago

Don't worry, i also spend few days debugging the lib to understand that was a target compilation problem. I think es2017 is not the only target that works, but should be the latest.

I can't pretend that i maintain the project by my-self, because i must spend more time on it. But yes, recently @timkrins and me have been promoted maintainers.

I want to merge some PR (including mines), but do to that properly, i should release a new version of the lib on NPM. Don't think i have rights for that for now.

We want to upgrade the TS version of the lib, but that need some work and some TS expertise. I'm waiting for a refactor period at my work and i will give more time to Spraypaint lib, promise !

corrspt commented 2 years ago

Ah, thanks for the explanation. That makes sense, if you don't have the publish rights. I'm closing the issue then.

Although, I've targeted a github branch directly in package.json before, might be an option to merge some of those PRs (without publish rights).

In any case, thanks a bunch for the help. I'll continue evaluating the library, seems like a perfect fit for what I need. If I see something that I can help, I'll try to submit a PR as well.

Best regards

corrspt commented 2 years ago

Hey @bilouw

Sorry for asking for help again, I'm struggling with a related issue, maybe you can tell me what I'm doing wrong.

The first one is, I can't save relationships (hasMany or belongsTo) with a .save() only if I do .save({ with: 'relationName' }) otherwise I can't make it serialize the the relationship value (and if I do thewith` part it also includes the relation object in the included section).

The second one is that in a hasMany relations, spraypaint is only sending the diff, but per (https://jsonapi.org/format/1.1/#crud-updating-resource-relationships) if I send an array, the server should consider that a total replacement. I have an example with an Account that hasMany Features ... If I load the account with 3 features, and add a fourth one... spraypaint when saving will only send the new one... and the server will replace all of them with the new one. If I delete two of the features (total after is one).. spraypaint does not send any feature in the relationship :(

Any ideas or pointers are much appreciated, thank you very much.

GastienneLea commented 1 year ago

Hi @bilouw , I have the same issue as @corrspt , i have already change the target and the module but still the same. Thanks