-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RFC] JSDoc Script Attributes #1148
Comments
How would it work with things like default values, min/max and assetTypes? Can it also handle the JSON attribute? There's also some requests with the existing system where users want to share common data between scripts like an enum listing of colours or a particular JSON format for JSON attributes. Would that work with the proposal? |
Somethign along the lines of this /**
* @attribute
* There are additional tags an attribute can have to provide more context to the editor.
*
* Range, step, precision specifies numerical constraints
* @title The title tag is used as short label
* @range [0.1, 10]
* @precision 0.01
* @step 0.005
* @placeholder This is some placeholder text, used to populate input fields
*/
velocity = 2 https://gist.github.com/marklundin/99ee9174bdb8fc37723df15e7d9ab826#file-my-class-js-L71-L76
You would be able to import enums and other types from other files in the asset registry |
Hi what about TypeScript support ? |
yep, this will be compatible with TS. |
Any thoughts on this @kungfooman @Maksims |
@marklundin does this include the nested seralizable objects? |
It does, but this isn't included in the example. Will update |
It does sound pretty nice, with a few caveats (good to mention in the first post as well), as drawbacks vs existing or other potential solutions - are as important as benefits when defining new APIs.
|
Thanks @Maksims, good to have your input.
The JSON type becomes a complex type, this could be defined through the jsdoc
Yep, events on attributes become opt-in and up to the user. This makes it clear and transparent to the user and solves the getter/setters issue. Additionally non-attributes may want events, so this separates the two.
Not sure I follow. Could you give an example?
Yeah we'd definitely want to improve the experience in Monaco to add highlighting, rollover annotations etc. A key feature here is that we would win regular attribute intellisense/autocomplete
This is simply to support json type uses cases, so a
Do you mean like assetType? These would be additional tags like
Yes enums would need to be defined elsewhere. |
A side question, how will these attributes look in typedoc? Will they be among other properties/accessors? |
Tags like type and enum are already so would be handled as-is. It looks like typedoc supports custom tags but not sure what control you have over generating output |
To also respond to some of @Maksims questions:
Events become your responsibility to implement as and when you need them. Less custom magic under the hood - you work with plain old esm/es6 (which everyone knows and understands, hopefully).
I don't imagine users struggling in the general case adding plain es6 members (though we might want to think about the custom jsdoc tags). In return for this change though, the script attributes themselves are included in auto completion and type checking - quite a big win. |
Coming back to this question :) |
Hmm, I maybe missing something, but since this is a module, we can now just do // settings.mjs
const vehicleOptions = { isDynamic: true };
export { vehicleOptions };
// script.mjs
import { vehicleOptions } from './settings.mjs'; Unless you mean something else. |
As a good exercise when designing APIs, please show code implementation of these examples: Complex JSONvar Example = pc.createScript('example');
Example.attributes.add('chunks', {
type: 'json',
array: true,
schema: [{
name: 'name',
type: 'string'
}, {
name: 'shader',
type: 'asset',
assetType: 'shader'
}]
});
Example.prototype.initialize = function() {
for(let i = 0; i < this.chunks.length; i++) {
const chunk = this.chunks[i];
this.material.chunks[chunk.name] = chunk.shader.resource;
}
}; Enum and Attribute Eventsvar Example = pc.createScript('example');
Example.attributes.add('dimensions', {
type: 'number',
default: 1024,
enum: [
{ '1k': 1024 },
{ '2k': 2048 },
{ '4k': 4096 }
]
});
Example.prototype.initialize = function() {
this.on('attr:dimensions', () => {
// regenerate texture with new dimensions
});
}; @slimbuck how does an attribute event work with new attributes, please show an example. |
Since there will be no built-in events user must add them if they need them. Users can go as complex or as simple as required, but we imagine most devs will probably just implement an attribute setter and manually evoke an event, something like: class A : extends EventHandler {
_myVar = 1;
set myVar(newValue) {
this._myVar = newValue;
this.fire('myVar', newValue);
}
get myVar() {
return this._myVar;
}
}; If this is ultimately too cumbersome, we can also consider adding helpers for creating standard getters and setters like the script system did before (but then you'll be left without auto completion and type checking). In any case, the user has control. |
@slimbuck code above does not have |
Yep this would work, enums and types can be exported and imported across files |
@Maksims Coming back to you earlier point; var Example = pc.createScript('example');
Example.attributes.add('chunks', {
type: 'json',
array: true,
schema: [{
name: 'name',
type: 'string'
}, {
name: 'shader',
type: 'asset',
assetType: 'shader'
}]
}); can be represented as the following /** @interface */
class Chunk {
name = ''
/**
* @type {Asset}
* @assetType {Shader}
*/
shader
}
export class Example {
/**
* @attribute
* @type {Chunk[]}
*/
chunks
} The |
Looks like there is almost full feature parity between old-way and JSDoc-way. Will it still be possible to use the old-way? As it will make code migration much easier and some still might prefer it. |
I would like the JSDoc parsing using Babel and/or TS (for type compatibility with IntelliSense) to be open-source, but otherwise I like the JSDoc way! /**
* @type {Asset}
* @assetType {Shader}
*/ For such simple types I would opt for one-liners: /** @type {Asset<Shader>} */
|
|
@Maksims Good point and then it would be a string here too: /**
* @type {Asset}
* @assetType {'shader'}
*/ My point is to have better typing overall aswell: TS Playground Right now everything is just A little bit of typing and we could know exactly based on type inferring: And auto-complete works aswell: Considering /** @type {Asset<'font'>} */ And people can write that using IntelliSense aswell: IMO too good of a chance to miss out on here while we are already refactoring. |
Looking at JSDoc, these brackets: So the @assetType keyword, is purely instructional filter for Editor, so it can filter assets when picking using attribute. It always will be Asset, with whatever resource inside. So it will be not within JSDoc style to do so. |
I believe your argument is based on lack of type knowledge when it comes to template types or some confusion around it. Lets make things clear, a Simple example: const map = new Map([['a', 1], ['b', 2]]);
const mapIterator = map[Symbol.iterator]();
mapIterator; There is your
The "actual type of a thing" is exactly what I used - a literal string type. That's as good as a type as any other type. Example of two instrinsic string templates and the stock /** @type {Uppercase<'hai'>|null} */
const test1 = 'HAI';
/** @type {Lowercase<'HAI'>|null} */
const test2 = null;
/** @type {Record<'someKey', 'someValue'>} */
const test3 = {someKey: 'someValue'}; |
Some guys write the code locally, using TS. After which the code gets compiled, minified and gets uploaded. The compiler will remove all those comments in that case. |
Asset.type - is not a js type of Asset.resource, nor it is 1:1 match even (text, css, html, types, etc).
If some notation requires some niche knowledge of templating in JSDoc notations, or some other languages - this will keep users from using it. Please, keep things simple. |
Not sure, but the |
Niche knowledge about a PlayCanvas-specific JSDoc tag spanning 4 lines is better, easier to read/write/learn than a one-liner using standard container typing? |
There is no 1:1 match of assetType with actual type. This is purely asset picker filter option, that is a string, and does not affect actual object inside of it.
We can make crazy long lines - and that won't make it easier to write.
Now we go through
/**
* @attribute
* @type {Asset}
* @assetType shader
*/
chunk or /**
* @attribute
* @type {Asset<'shader'>}
*/
chunk I'm afraid IDE wont be able to help here user unlike with @assetType. And actual attribute object will not match the type of a thing. There is no existing template use that matches such a pattern, so this will not be intuitive to users, and will go against actual pattern: defined type - is an actual Type or array of Types returned. End of the day - please, keep it simple, and do not overengineer with the assumption that other users know your particular style and preferences. Less prerequisite knowledge for it - is better. We can argue about this for ages (reminds me of these nice old days of arguing about variable names :D), end of the day - it has to be simple to users, not for you specifically. |
Ideally we'd have a type in the engine that mapped to each asset.resource. Also I love the one-liner The fact that |
Quick revision on the complex type for nested attributes ('json' type in existing code). We're proposing this syntax /** @interface */
class EnemyType {
power = 10
/**
* Additional constraints supported
* @range [0, 1]
*/
speed = 3
}
export class MyScript {
/**
* @attribute
* @type {EnemyType}
*/
enemy
}
Why?Using the type and interface tags provide greater flexibility to define sub types using standardised notation.
/** @type {{ power: number, speed: number }} */
/** @attribute */
enemy = { power: 10, speed: 10 }
/**
* @typedef {Object} EnemyTypeDef
*
* @property {number} power
* @property {number} speed
*/
/** @type {EnemyTypeDef} */ Each of these notations support Intellisense out the box with editors that support JSDoc tags. Any thoughts on this @Maksims @kungfooman @LeXXik |
No, no it isn't. And that's why I agree with @Maksims that this is a mistake.
|
We've opted to use a tag for the asset type, but the camel case of /**
* @type {Asset}
* @kind texture
*/
myAsset |
I don't like the idea of hijacking |
In the current Script system we declare attributes using the
static attributes
propertyCurrent Scripts
ScriptType class
Using class based syntax, this can be declared like this...
Issues
attributes
tag in order for this to workmin, max, precision, step
are just for the editor to provide additional information around presenting UI. They are not required byt the class itselfProposed inline attributes
This RFC proposes an alternative way of declaring attributes using JSDoc style comments
There are a few things to note here:
@attribute
modifier tag@type
tag@range
,@title
etc. A full example can be found hereFeedback
Any thoughts and feedback on this would be massively appreciated 🙏
The text was updated successfully, but these errors were encountered: