decahedronio
some ideas
Hey guys !
First of all, great library you got here.
I disagreed with you on some points, mainly the fact that you choose to implement annotated aliases and case conversions (which are awesome features tbh !), although you couldn't implement type checking on the JSON payload in the .fromJson() method because of this choice.
So I forked it on insurgent-lab/ts-entity to adapt it as I wanted it to work.
The following is a part of my README.md, you can find links to Typescript Feature Requests issues that will allow me to re-implement those features without breaking type checking (if they are ever implemented and released):
Disabled features (from fork)
These features were disabled because they were blocking the type validation (which is the whole point of Typescript). We are now waiting for Typescript to implement proper ways for us to do that (see linked Github issues).
- Disabled annotated aliases, as it is currently impossible to mutate the type using a decorator (Github issue)
- Disabled snake_case <=> camelCase conversions, as it is currently impossible to mutate type keys with a MappedType (Github issue, similar case comment)
Also here are some commits that might interest you:
-
chore: add .gitignore, yarn.lock and missing package (you're still missing
reflect-metadatain thepackage.json) - build: add semantic-release for automated releases (this can help you release automatically with patch notes if you just follow the Conventional Commits :smile:)
- feat: add JSON data type validation
The last one that enables JSON data type validation also includes the following breaking changes, so I doubt you're interested in it but we never know :wink:
BREAKING CHANGE: Disabled annotated aliases
BREAKING CHANGE: Disabled snake_case <=> camelCase conversions
BREAKING CHANGE: .toJson() signature from toJson(toSnake: boolean = true, asString: boolean = false) to toJson(asString: boolean = false)
I also made a big rebase of the whole commit history to make the commits compliants with Conventional Commits, and fix some wrong SemVer version bumps (you should currently be at 1.7.2).
That's it, just wanted to share those infos with you as payback of the great work you did ! :1st_place_medal:
Thanks again !
Hey @SherloxFR! Firstly, thank you very much for your effort to document all this, this is great information, and I'm happy to see how others use this library.
As you rightly state, we have no interest in disabling the camel conversion 😉. Just for clarity, the conversion is there because we prefer working in camelCase, while APIs are generally snake_case. Now, as you point out, this means you can't type check the input. This is generally not needed where I've used it as it's being passed from an API response (which cannot be type checked since the data is not available at compile time).
However, if you wanted to just use snake case, you can simply set the static property enableCamelConversion = false. I'm not sure whether this would solve your problem of type checking?
As for the to-do I noticed in your readme, about getting rid of the need for = null, you can do that already; any property that is @Type decorates does not need an initial value (as the property name is stored in the metadata storage, we don't need to retrieve it from the prototype at runtime).
Automated releases is something I've been intending to do for a while but haven't gotten around to, so I might steal that bit off you 😄
P.S: yarn.lock isn't committed because it is generally discouraged to commit lock files to packages, as the consumer is meant to be resolving dependencies based on versions that match not just a package, but resolve a package that satisfies version constraints of all dependencies. e.g if your package's package.json requires ^3.4 of a dependency and is version locked to 3.4.2, the package consumers won't be able to upgrade that dependency, and your package will be incompatible with packages that rely on ^3.5 (even though, as per your package.json, they are compatible).
Hey @phroggyy, thanks for the quick reply :smile:
I'm working with camelCase as well, and my goal right now is to interface with a MySQL database, where datas are stored as snake_case. So indeed the feature is interesting, but simply setting enableCamelConversion to false wasn't possible because the problem was in this line (after enabling type checking):
key = EntityBuilder.enableCamelConversion ? StringHelper.toCamel(key) : key;
It wasn't even possible to compile due to an error like Extract<keyof T, string> is not assignable to type string.
This feature is really important to me, so I'm still looking into a way to re-implement it via an helper function while TypeScript #12754 is still WIP.
About getting rid of the need for null initialized properties, I actually would like to get rid of the need to annotate them with @Type as it is already declared so Typescript pick it up.
Working:
class UserWithAnnotatedPosts extends User {
@Type(Post)
public posts?: Post[]
}
Not working:
class UserWithAnnotatedPosts extends User {
public posts?: Post[] = null
// error: References to classes must be specified via @Type decorator
}
This is totally OK as we can't get the class to build at runtime if we do not annotate it anyways.
My concern is about working with primitives:
Case 1: Not annotated / Initialized
Working (jsonParse > if (sourceObject.hasOwnProperty(key))):
class UserWithAnnotatedPosts extends User {
@Type(Post)
public posts?: Post[]
public value: string = null
}
Case 2: Annotated / Not initialized
Working but it's weird to annotate with no specified type (jsonParse > if (metadata)):
class UserWithAnnotatedPosts extends User {
@Type(Post)
public posts?: Post[]
@Type()
public value: string
}
Case 3: Not annotated / Not initialized (this is what I would like to implement)
Not working:
class UserWithAnnotatedPosts extends User {
@Type(Post)
public posts?: Post[]
public value: string
}
I opened an issue about this on my repository if you want to track my progress on this.
I'd be really happy if the automated releases + release notes could be of use for you :smile: It's something I deem mandatory on every project, alongside with Conventional Commits.
PS: The insurgent preset adds two commit types to the CC specs (task & improvement). task is not used very often as it is only for JSON hardcoded files updates or things like that. improvement is not shown on releases notes right now, but I'm thinking about this, it's a very recent work.
About the yarn.lock file, Yarn actually ignores the one in the node_modules folder (source):
When you publish a package that contains a yarn.lock, any user of that library will not be affected by it. When you install dependencies in your application or library, only your own yarn.lock file is respected. Lockfiles within your dependencies will be ignored.
I understand your use case, but unfortunately don't believe it's possible with the way TS works. As the decorator is executed at runtime, and TS won't include non-instantiated properties when compiling, I don't see a way to make this work, unless TS were to expose more type information to decorators (which has been requested).
Definitely let me know if you figure that out though!
Well actually :stuck_out_tongue: we can just use Object.keys, working on this rn I keep you updated.
@phroggyy well ...
Because Typescript removes undefined properties from class when compiling, the only option do implement this is using the useDefineForClassFields option as well as targeting ESNext (this may be fixed, see below) with the following tsconfig.json options: { "target": "ESNext", "useDefineForClassFields": true }.
Code example
class UserWithoutInitializers {
public name: string
public email: string
public daysAvailable!: string[]
}
const test = new UserWithoutInitializers()
console.log(Object.keys(test))
Compiling with useDefineForClassFields && target < ESNext
class UserWithoutInitializers {
}
const test = new UserWithoutInitializers();
console.log(Object.keys(test)); // Returns: []
Compiling with useDefineForClassFields only
class UserWithoutInitializers {
constructor() {
Object.defineProperty(this, "name", {
enumerable: true,
configurable: true,
writable: true,
value: void 0
});
Object.defineProperty(this, "email", {
enumerable: true,
configurable: true,
writable: true,
value: void 0
});
Object.defineProperty(this, "daysAvailable", {
enumerable: true,
configurable: true,
writable: true,
value: void 0
});
}
}
const test = new UserWithoutInitializers();
console.log(Object.keys(test)); // Returns ['name', 'email', 'daysAvailable']
but when building & running tests on the lib, it results with all
Message:
TypeError: Right-hand side of 'instanceof' is not an object
Stack:
at <Jasmine>
at dist/src/support/metadata/MetadataStorage.js:56:87
at <Jasmine>
at MetadataStorage.findTypeMetadata (dist/src/support/metadata/MetadataStorage.js:56:57)
at UserContext.<anonymous> (dist/spec/Type.spec.js:11:57)
at <Jasmine>
Compiling with useDefineForClassFields && target === ESNext
class UserWithoutInitializers {
name;
email;
daysAvailable;
}
const test = new UserWithoutInitializers();
console.log(Object.keys(test)); // Returns ['name', 'email', 'daysAvailable']
At least it works ! (All tests are passing with the last options couple)
I think I might find a way to make it work for target < ESNext, but this is not really a priority for me as I'm using ESNext on my projects anyway.
So all of this is implemented in https://github.com/insurgent-lab/ts-entity/pull/2. You'll definitely not be interested in this as it only works with Node 13.2+ (latest right now :joy:), but maybe with Babel it can work pretty much everywhere (not sure at all tho).
Also this might interested you (related to the automated releases): ci: implement Github Actions.