Remove defaultConfig + defaultUserState

[possibilities]

We don't document this but it's in the default template. I vote for removing all references to this for now until we decide what we want?

https://github.com/Versal/sdk/blob/master/templates/minimal/versal.json#L10

/cc @mereskin @janpaul123

[mereskin-zz]

I'm fine with that.

[janpaul123]

Let's decide what we want. I vote for keeping it:

1. We want some default state, and it seems logical to keep it in the gadget repo, so people can refer to it as an example. In fact, we could at some point generate a static example page from versal.json in the same way as the Polymer folks do with their components.
2. It seems logical to keep such examples / default state for gadgets decoupled from the actual gadget code. After all, people who instantiate the gadget should know how to config them, right?

[pkaminski]

FWIW, I think keeping defaultConfig in versal.json makes sense, but not defaultUserState, since the latter should be specific to a gadget instance.

[possibilities]

@janpaul123:

1. It's definitely nice if it's in a common location to point at as an example but you could also point at a js or json file in your repo with the default attributes. And we could generate an example showcase anyway... either a) it's baked in the gadget js and it'll just work or b) we can push it up from an included data/default.json (or many, e.g. data/rainbows.json, data/bw.json) to the api (even more flexible because you're not limited to one). Not sure I disagree completely but not totally convinced by your argument.

2. why not include a json file that the platform grabs and uses?

Also we having naming to deal with... they're both named poorly. My argument is that it's poorly designed enough that I'd rather deprecate it and introduce something explicitly ASAP. The gadgets made in the meantime won't have this entanglement and will be easier to port (or no less hard) to a better API later.

@pkaminski

I think it can be valuable when you want to init some value to a default irrespective of learner, like setting a counter to 0.

[janpaul123]

Maybe you're right @possibilities. We can delete them first and then think about what we want exactly, perhaps also based on what gadget developers do in their gadgets themselves.

[possibilities]

Removed [Discussion] from the title and keeping at "in progress" as I'll PR a solution to this today.

[janpaul123]

Cool, thanks!

[possibilities]

After spending some time attempting to implement the complete removal of these from the manifest and talking with @mereskin I'm feeling a lot less excited about this approach:

• Making the example gadgets work without this feature wasn't pretty so I added a setDefaultAttributes to player api. Adds complexity and there's no way to make it apply to only the very first attributesChanged.
• @merskin points out that what we really want is more of a declarative API like react's getDefaultProps. In the context of a web component launcher we can get this kind of functionality by setting attrs on the component. For an iframe launcher it makes less sense and probably setting defaults in the manifest is the most appropriate API.

At this point I don't think we should throw the baby out with the bath water. I propose adding a deprecation warning to the usage of both defaultUserState and defaultConfig in the manifest (on SDK upload) and introduce a renamed defaultConfig as defaultAttributes. This gets us sane naming and gets rid of learner defaults.

/cc @janpaul123

[janpaul123]

That sounds like a reasonable step for now. At least we have explored some different options here now, and we hopefully have a better idea of where we want to go.

[pkaminski]

👍