#V10-Document-Model-RFC

1 messages · Page 1 of 1 (latest)

mortal tinsel
glossy bramble
#

Ah finally more data.data breaking stuff, I was very disappointed by v9 in this regard thisisthetext

bleak patio
#

I will definitely take the time to read this in detail but from a first glance, I quite like it. The circular references are something that is extremely hard to model correctly in TypeScript, so I would be very happy if that is simplified.

rough eagle
#

I'm strongly in favor of making this change, in particular because of the repeated issues and mistakes surrounding data.data.
however, I'd like to suggest rethinking the suggestion of "system", to be something slightly more descriptive like systemData.
(I'm referring to this:)

- actor.data.data.attributes.strength = 10;
+ actor.system.attributes.strength = 10;

my concern is that "system" sounds like it should contain the name of the system the actor belongs to, or stuff like that; as opposed to the intended meaning of, well, system data fields

steel swan
#

I don't really have too much to add than "YES. PLEASE."

The data.data in particular have been a great confusion. Especially if you work around it in some places but forget in other places.

Yes, I'll need to rework a lot of code, but this will make it infinitely more readable.

proud ridge
#

my inner grinch hates all kinds of change, but a clearer distinction where what is Foundry data and what is system data does seem benificial in the long run

burnt scarab
#

I would also very much welcome these changes, getting around to use actor.data.data amongst other things would make it so much more readable / understandable. I really like that data then becomes system, making it much clearer what it actually refers to.

tender merlin
#

I just got done with my initial read over and I strongly approve.

I know the number of warnings in the console could create some support hell until major systems and modules make their adjustments, but I know that would be a temporary bump in the road that would be well worth it in the long run.

visual grove
#

As a system dev, I'm going to agree with the suggestion of moving data.data to system, with the known rub that is would break downgrading to v9 as it renames the data field. I'm not quite sure that is the best idea right now with that thought in place.

paper pike
#

+1 to systemData over system as it's more explicit

visual grove
#

Odds are, it could be shimmed in V10 to support both, but only use data on new documents, then v11 can make the move to system or what ever the new prefix is.

tender merlin
amber dew
#

Yeah, after reading the RFC, Spices concerns about support were also my immediate thought, people will need to be very intensely reminded to take a backup of their data so they will have something to return to since downgrading will not be immediately working.
But my inner dev does likes the changes proposed.

visual grove
#

At least I've made a tool that us Mods have access to that would be easy to adapt into downgrading a move like that.

mortal tinsel
#

It's going to be very difficult to support moving back to V9 (or older) from a world which has been migrated to V10 - for reasons other than this particular change as there are other one-way data model migrations in V10

#

We don't have a good mechanism for going backwards except to restore from backup, and I worry the cost of trying to engineer one might be prohibitive.

visual grove
#

Smash everything then!

burnt scarab
#

ui.notifications.error("MAKE A GOD DAMN BACKUP BEFORE UPGRADING!!!", {permanent: true}) 😉

visual grove
#

Take it as an opportunity to make changes to the data model that would be drawn out over some number of versions.

tender merlin
#

Yeah, that makes sense. It would be nice, buuuut it is what it is :)

I would definitely make sure there is a great big "BACKUP YOUR DATA BEFORE UPGRADING! HAVE YOU BACKED UP? SERIOUSLY, BACK UP!" message to the upgrade dialog :)

mortal tinsel
green flower
#

I'm all for it if only for this one reason

- document.data.data.*
+ document.system.*
- document.data.*
+ document.*
tender merlin
#

Yeah I love that

jolly tree
#

I hate it. I'm sorry, but for all the good it does, I'm used to data.data and my time to fix stuff is both, precious and limited. I suspect that using replaceAll in my IDE won't do the trick and I'd have to spend a significant amount of my free time to adjust to it. On top of the other changes I'd have to take care of. And then the modules which are only slowly updated on which youd have to wait. Nope, don't like it at all.

visual grove
#

If we can have enough shims in v10 that most v9 code works as normal, I'd like that as I like to straddle 2 major versions.

mortal tinsel
#

There are good comments about system (more terse) vs systemData (more clear) - we would want to think about that before committing, although it does not alter the spirit of the proposal.

wispy ibex
#

The changes sound pretty reasonable and desirable to me. Though the wiki's definitely gonna need a page on system updates for V10 to help devs out.

And I do share concerns regarding the one-way update, given the number of issues that sometimes causes

visual grove
paper pike
#

How does this affect temporary documents which do not exist on the database? Would apply be the only way to update their data, or would update be shimmed to redirect to apply for local-only documents?

tawny depot
crimson spire
mortal tinsel
visual grove
#

Threw a comment on the GL issue about moving .data for system data to .system/.systemData so it's not going to get lost.

woven pulsar
#

As a developer at the "Code Monkey" level of competency, I had great difficulty adjusting to the changes made from v7 to v8 and only resolved them once a guide had been published in the wiki and I had sought some specific assistance. The changes from v8 to v9 were more subtle but rose up to bite me unexpectedly and are still not completely resolved (migration of system compendia for example).
That said, I am completely in favour of the proposed changes for all the reasons you state in the document.
However, I would ask that there be a transition guide created to help us understand what changes will be required of us as a result. (I do note that there were some tips in the document that would be a good start).
Thank you for creating this wonderful, complex and configurable foundation upon which we can build our own dreams.

rough eagle
#

a transition guide would be a must, and it will be particularly helpful if it also includes a basic script to resolve 90% of the migration (e.g. highlighting every usage of data.data in your module code, or in every macro in the world)

green flower
mild current
#

@mortal tinsel There's a date discrepancy between the GL issue and your original post here, in regards to the release of Prototype 1

wispy ibex
mortal creek
#

Kinda doubt it.

stiff cliff
#

While it's somewhat frustrating to refactor the same things multiple times, I would not miss the data.data thing.

green flower
rough eagle
#

even so, there is a lot of value in creating an automated tool that tells the developer exactly how to migrate (and maybe even edits files for them).

additionally, I think actual users will want something like this for their macros. migrating dozens of little unique snippets of code in every foundry world is a big problem that doesn't only involve module/system devs.

wispy ibex
gloomy jetty
green flower
#

I don't really disagree, but he specifically listed the example of highlighting every usage of data.data which doesn't need more than what he typed out in the question

wispy ibex
tawny depot
#

The main issue with find and replace is when you have times you are referring to e.g. ActorData, so the system data is at thing.data, not thing.data.data (and such a bit of code will need refactoring anyway for this). Find and replace and check for errors is a decent first step, but only a first step.

stiff cliff
mortal tinsel
#

I think that a more reliable approach than "find+replace and check for errors" which could introduce unexpected side effects which are hard to identify and catch is to instead force the behavior that you want to eliminate to fail (or log a warning) and then test your system until you have caught and eliminated the problems.

#

Given that the old data.data path would continue to work (with a logged warning) it removes the time pressure to fix it immediately

stiff cliff
#

I think the main technique is going to be chasing deprecation warnings.

gloomy jetty
#

After reading the issue, I agree it's a good step forward. A tad painful maybe, but worth it on the long run. data.data will not be missed, and it would simplify the code internally and also externally (modules/systems and TS types)

mortal tinsel
#

you can just whittle away by replacing any usages over time when you see them show up in the logs

stiff cliff
#

That being said, I have had entity/document issues that got missed for multiple major versions before someone got an error and reported it.

#

So just chasing warnings isn't enough unless you have unit testing already set up so that you can ensure every control path gets run.

grand charm
#

Want me to be frank? Another version another months of fixing things in huge systems like 3.5e, probably breaking all my premium module importers and other utility mods. The deprecation path does not help - the problem is the approach to breaking stuff. It means I have to constantly spend time on upkeep, not on developing new things. If you to do giant refactors each time new version of Spring/Angular/Django/Windows/MacOS etc appear you would be infuriated. Here it seems like it’s norm.

stiff cliff
#

You can always make the redirect permanent.

#

I think that's how most of the listed frameworks handle it. They just maintain the deprecated paths for a loooong time.

mortal tinsel
grand charm
stiff cliff
visual grove
tender merlin
#

I can't count the number of times as an Android & iOS dev back in the days I had to update my apps due to changes new versions of the OS's would bring

grand charm
#

Yeah, I will see myself out for wanting to have stable APIs and not spend month looking for each tiny small log message in the limited free time I have.

stiff cliff
plain loom
#

This will absolutely sink Sandbox for a long time. With every new core version I have struggled to find the time to update it. I am no coder, and it costs me tons of effort to keep the system updated. This change in the data system is massive in comparison with the previous changes (from my system point of view), and to be honest, the number of constant changes and need for updates is starting to be overwhelming. Apologies, but is how I feel about it

amber dew
#

I know that refactoring feel, but consider doing this while being paid for it is different than having to spent your own free time on doing it.

grand charm
#

There were jokes here where V8 landed that "Yeah, in V10 we will do DocumentArchetypes". The thing is, it stopped being jokes... basiliSad

woven pulsar
stiff cliff
#

The question isn't "do breaking changes suck" the question is "do the benefits outweigh the consequences"

stiff cliff
mortal tinsel
mortal tinsel
grand charm
#

I have 12 premium modules, and 3.5e system to update now. If I keep release schedule I will have 24 next year.

mortal tinsel
grand charm
stiff cliff
grand charm
#

This is 3.5e

gloomy jetty
#

Chesus

mortal tinsel
#

I do understand that its pervasive - but I do want to clearly re-emphasize that this part of the proposal is non-breaking. It will continue to work as is without any change.

stiff cliff
#

To be fair, this probably indicates that I should be passing the inner data around in my data flow instead of referencing it like this all the time...

proud ridge
#

so until v11 this will just give deprecation warnings?

visual grove
#

Yes.

mortal tinsel
#

compatibility would be preserved much longer than just v11 (if not "forever")

#

and we are providing a way to disable the warnings

stiff cliff
#

Yeah, so let's talk about the actually breaking thing.

Anyone got system data keys that will conflict?

grand charm
#

Cause last time Deprecated meant "until next major"

#

And that is why I panicked, cause it always was like that until now.

plain loom
# mortal tinsel Thanks for sharing your perspective. Would it be possible for you to provide a c...

Both references for Sandbox specific items and the collisions. Right now Sandbox creates info and templates using the data.data structure, building things from scratch, so not as easy as to do a replace. But the thing is it also creates new unique ids that are stored in the document, so I will need something to ensure the integrity of old Sandbox worlds by moving this ids to the new system data. I am not saying is not possible, is just that it will take me some time to analyse and assess

visual grove
#

Yea, no note about how long Foundry will keep the compat shims.

grand charm
#

And for that change 6 months (or 8 months if I can start before) is too little in case of 3.5e

glossy bramble
#

I think it's ok like it is but just an idea

mortal tinsel
#

That's a fair call-out, we should have documented the proposed compatibility timeline more clearly. I would envision if we make this change we would keep the backwards-compatible shim around indefinitely or until we are confident that the impacts of removing it would be very limited to a few holdout packages.

grand charm
#

cespenarShiny That sounds a lot better!

stiff cliff
#

You can always grep through all existing packages for data.data 😆

mortal tinsel
#

we would certainly encourage any new code to be written using document.systemData rather than document.data.data though.

grand charm
#

Thats fair!

stiff cliff
rough eagle
# 
    globalThis.logger.warn(`You are accessing the ${this.constructor.name}#data object which is no longer used. ` +
      "Since V10 the Document class and its contained DataModel are merged into a combined data structure. " +
      "You should now reference keys which were previously contained within the data object directly.");

I imagine this log message will be shown in the thousands to end users who had no idea this change happened and just used an unmigrated macro. I suggest adding a URL, logged or in a comment next to this, that links directly to a short migration guide (i.e. not this entire RFC).
I'd like non-techy players who look at the console log to be able to see this message and then fix the data.data in their macro within a few minutes.

stiff cliff
#

Also effects Macros

grand charm
#

Ah my bad dodgerShrug My reading skills are failing me then atrakaLOLD

viscid mist
heavy basalt
#

I like it overall, but I definitely think the compatibility shim should stick around as long as is feasible to help cut down on maintenance work devs need to make.

Something else that I also saw in the V8 transition was that it was difficult to piece together what to look for aside from helping people understand the warnings in their console. Something like a well-commented diff of the changes needed to get Simple Worldbuilding (and/or my Boilerplate system) working in V10 could be a huge help to get people started with where to look for their updates.

visual grove
#

I'd ask for more aggressive deprecation warnings as time passed, mild logging in V10, warning popup one time in V11, get's an error popup v12, then a large permanent pop up in the last version that supports it. Space the ramp up as practical.

tawny depot
#

How will this affect active effects which use e.g. data.attributes.foo.bar as a key? (Where data here is the system data). Those would also presumably need a migration

mortal tinsel
#

could also affect things like configured token bars

bleak patio
#

It also affects custom effect alternative solutions…

#

Like the rule elements that are part of pf2e

white topaz
# heavy basalt I like it overall, but I definitely think the compatibility shim should stick ar...

As someone who started with little Javascript knowledge and only wrote some macros with a bit of help, fixing them myself felt kind of impossible, since I didn't even know why it broke or what broke. It just told me "This isn't working", but no idea how to replace it.

And the depreciation warnings generally weren't too helpful to me. I do know a good bit of Javascript now and could fix some, but most fixes I had to ask others because it wasn't obvious why things didn't work anymore.

Since the depreciation warnings were only shown in the console, and if I didn't check there, I'd never know my macro would break in a new version, since it's working just fine currently

tender merlin
#

The first launches of v10 will have so many warnings in the console 😅

visual grove
#

Yea, I'm going to quash thread jacking here. Submit your own issue on the GitLab.

mortal tinsel
#

I believe, but need to test, that un-migrated active effects or token bars etc... would continue to work

#

But providing a migration for them would be preferred.

stiff cliff
mortal tinsel
bleak patio
fathom folio
#

In the end does this mean that the structure and methods across actors and settings will be the same or just the object data subject to this change? I suppose I am asking if this also means there will be similar ways of saving data too, i.e. saving actor data vs saving data in a custom settings form?

bleak patio
#

How will changing the document about to be created in preCreate hooks work with this change? Currently, the intended way is to call update on the data passed into the hook callback. With this change, that doesn’t exist as a separate thing anymore. So how will that work instead?

tawny depot
#

In an ideal world for deprecations, I would want getProperty(actor.data, "data.foo") to return actor.system.foo - I'm guessing how well this works is down to how exactly the deprecation is handled, but wanted to be sure it was flagged. (It would currently break Memento Mori, though that's an easyish fix if we don't get this)

mortal tinsel
visual grove
heavy basalt
mortal tinsel
stiff cliff
mortal tinsel
# stiff cliff I'm a little confused on the semantics of `apply` vs. `update`?

As with anything that involves needing to choose a name for something (like system vs systemData) - we are open to discussing and iterating on feedback. We chose apply because we felt it was the best available option which did not collide with update (which is reserved for actual database modification operations i.e. create/update/delete).

stiff cliff
mortal tinsel
#

you can join Calego's bandwagon 😛

stiff cliff
#

I usually do.

#

Is that technically part of this discussion, or a side topic?

mortal tinsel
#

I would say side topic (not to try and dismiss your question)

stiff cliff
#

Should there be another GL issue regarding it?

mortal tinsel
#

There is one (again, Calego's bandwagon)

#

As it relates to this conversation, however, there is an important technical requirement to have separate methods for (1) updating the data model locally in memory and (2) committing a change to that data model to the database. What is negotiable is what to name each of those methods.

unique parcel
#

Oh god, I come home from work, get two pings and this bombshell. 😄

surreal geode
#

I'd prefer the deprecations be phased in, across 3-5 versions...
Phase 1 (v10): Shim data and data.data as proposed, but make the deprecation warnings silent by default (or maybe just a single warning the first time it happens). Add in an easy way to globally enable these warnings by developers, and make sure there is a call stack included in the log.
Phase 2 (v11 or v12): Make the warnings noisy by default, with an easy way to temporarily silence them (so developers trying to fix other, more urgent issues can have cleaner logs).
Phase 3 (v12, v13, or v14): Remove the shims entirely - don't allow modules and systems to indefinitely use the old way, force an eventual upgrade to a single standard. It's no fun trying to add support for a new system only to find it doesn't follow the same standards as other systems, and it makes it harder for a developer of one system to jump in and help another system's developers.

#

Adding in migrations for macros to the new standard would also be hugely beneficial to non-developers who just copied macros from somewhere and don't understand how they work

green flower
bleak patio
gloomy shoal
# mortal tinsel Yeah, I've been a bit concerned about that and will be anxiously awaiting some i...

@keen rain will be awake soon. My gut feeling is that it will hurt in the short term but long term is preferable. The difference between us and most systems is that we can have a few people go and start sorting through, and if there are deprecation warnings we have the manpower to work through it if it is not a breaking change better than, well, literally everyone else - we simply have more “coding resources”. And given upcoming system evolutions I’d anticipate more people becoming involved with the project in the next few months and this is a good “learn the code base” one

stiff cliff
# green flower So `apply` is an in memory update whereas `update` saves the update with the ser...

My understanding is that there are two kinds of things:

  • Documents that exist in the DB
  • Data Models that exist only in memory.

(or maybe the naming is the reverse of that?)

But with this proposal, both will be usable as "a structured data object with a schema" but one will be modified by an update method, and the other by an apply method.

If my understanding is incorrect, then my thinking on this may change.

bleak patio
mortal tinsel
#

Both types of data model may be modified (locally) via .apply(changes). Only a Document which can be persisted to the database can be called with .update(changes) which are pushed to the server and written to disk.

bleak patio
#

With the suggested changes, both classes become one, but there is still a need for both methods. So DocumentData#update becomes Document#apply

mortal tinsel
#

that understanding is correct

bleak patio
#

maybe a slight amendment: there can be DocumentData subclasses that are not Documents, so those would not have an update method, just an apply method

stiff cliff
#

From the perspective of systems and modules though, needing to know that one data model does or does not have a database backing and another does seems... like they need to know the underlying implementation too well.

#

In other words, if I call update I expect the document to know what to do, but I expect the result is that the data is stored appropriately.

#

So I guess for a local data model, update should be an alias for apply and for a database document, update will apply then also commit the change to the DB.

bleak patio
stiff cliff
#

My code shouldn't need to know if the data model persists or not.

bleak patio
mortal tinsel
#

@stiff cliff the problem here is that there are use cases where you want to make a change locally but not persist to the DB. so having persistence "just work" for Documents would deprive you of that option (perhaps without some special parameter)

plush lake
#

Just so I understand, from a user perspective if I purchased a large scale campaign like Rise of the Drow to the tune of $70 (or any expensive premium content for that matter). Would there be a point where assuming the dev doesn’t keep it updated, it would become unusable or inconvenient to use upon a Foundry update because of this change? And if so, when?

mortal tinsel
# plush lake Just so I understand, from a user perspective if I purchased a large scale campa...

It's certainly possible that a particular package will cease to function as Foundry VTT evolves regardless of the topic of this particular proposal.

The design of Foundry VTT as a self-hosted software provides the user with freedom to choose which version of the software they want to use, allowing them the option of remaining on their current or preferred version indefinitely if they wish.

Certainly we don't want to force changes which break packages (premium or otherwise). As it relates to this particular proposal, it is unlikely that Rise of the Drow would be affected.

tawny depot
#

Would it be possible to have something like document.update(data, {commit: false}) for a local only update (which can default to local only for things that aren't present in the database, and to committing to the database for things that are in the database, and throw if commit:true is specified for a thing not in the database), which would be vaguely analogous to the temporary option for document creation.

stiff cliff
night lintel
#

Could I also suggest maybe sysdata instead of systemData. Only because I'm lazy and would rather not type in extra letters if I don't have to. 🙂

bleak patio
sick sparrow
night lintel
#

info instead of sysdata? Lol.

frank radish
#

😬

#

systemData can be autocompleted and is very clear 😉

mortal tinsel
#

.stuff

fathom folio
river estuary
#

Speaking more as a consumer of premium products than a developer I do worry the rather frequent large-scale changes to the API are going to be scary to prospective premium partners? From scanning through the above it seems there's a plan for a long-dated support period at least, but it probably doesn't calm nerves on an unstable API for investment purposes.

fathom folio
#

But at any rate, this change is pretty fundamental, how long of a "transition" or adoption timeframe would we potentially be looking at?

mortal tinsel
#

We discussed .commit(), which makes sense from a naming perspective, but we think that create, update, and delete are such standard verbs for these operations though that I'm reluctant to move away from that not to mention that Document#update is much more widely used than DocumentData#update was, so that would be a more impactful change which - for me - wouldn't be merited.

unique parcel
mortal tinsel
sick sparrow
fathom folio
#

I do rely on system settings a LOT (for the Cortex Prime system). And I think a lot of this will help as I have multiple lib files for helper functions, i.e. different version of my helper functions, since system settings and actor sheet JS changes are currently handled so differently

delicate rose
#

I am for this, but for some reason the name systemData bothers me. Don't know why... I think Atro has just trained me that Foundry properties should be terse. This coming from the guy who writes the longest variable names known to developer-kind

mortal tinsel
#

As an analogy, another change coming in V10 is some rationalization/organization of attributes of the Scene data model which pertain to grid configuration. Variables like gridType, gridSize, gridColor, etc... are being grouped as an inner data object with the following inner schema:

grid: new fields.SchemaField({
  type: new fields.NumberField({required: true, choices: Object.values(CONST.GRID_TYPES),
    initial: CONST.GRID_TYPES.SQUARE, validationError: "must be a value in CONST.GRID_TYPES"}),
  size: new fields.NumberField({required: true, nullable: false, integer: true, min: CONST.GRID_MIN_SIZE,
    initial: 100, validationError: `must be an integer number of pixels, ${CONST.GRID_MIN_SIZE} or greater`}),
  color: new fields.ColorField({required: true, nullable: false, initial: "#000000"}),
  alpha: new fields.AlphaField({initial: 0.2}),
  distance: new fields.NumberField({required: true, nullable: false, positive: true,
    initial: () => game.system.data.gridDistance || 1}),
  units: new fields.StringField({initial: () => game.system.data.gridUnits ?? ""})
})

The anology here, as it pertains to system vs systemData is - should we have named this attribute gridData as opposed to just grid. Should the reference path be scene.gridData.color or scene.grid.color. I would argue that the second reads more elegantly.

If that is the case, than why should actor.system.attributes.strength be viewed as less semantic than actor.systemData.attributes.strength. Perhaps it's not as clear cut as in the grid case.

#

I think there is a general argument to be made, however, that appending "Data" as a suffix should be superfluous unless the field name is too ambiguous without it.

foggy ledge
#

if you can name a thing that isn't data, then you can append Data to the name without it being superfluous.

mortal tinsel
#

A fair counter-argument. When you are no-longer within the scope of a dedicated data object where everything in here is data, it becomes more useful to differentiate

bleak patio
#

That doesn't mean I can't get used to the new notion

#

maybe we are also just too attached to data by now, so we don't want to let it go.... 😄

stiff cliff
#

If it were data.system this would be clear. But if it's just myThings.system it's a bit unclear what system is

foggy ledge
#

ah, yes, systemData not as in "data of the system" but as in "data provided by the system for the object", makes sense

bleak patio
keen hound
#

The argument I made when discussing this with someone else earlier was that for example for an actor you have
token
system
parent

Two of those are documents one isn't. Now which is which might not be clear at first glance but then again with how much stuff a token document has anyhow does it matter? Especially because a token presently has a permission field ( I actually don't know for what) that unlike actor.data.permission is a simple numerical value opposed to the usual object

I feel like this boils down to not wanting to let data go

foggy ledge
#

this makes very little sense in foundry context, but still

night lintel
#

I think if system already describes an object with System data in it... we shouldn't be duplicating names for different uses. There are enough words in the English language that something could be found that suits, without causing any confusion. Like, we've already trained ourselves to recognize system to mean one thing... we shouldn't be adding more things that it "could" mean depending on context.

mortal tinsel
#

well said

unique parcel
#

How about rulesData 😄

#

In the end a system is always a bunch of rules.

night lintel
#

I also love how a post about breaking changes has boiled down to a discussion on what exact word to use. 🙂

foggy ledge
#

preparedData since it's what you get after you run the prepareData method.

bleak patio
#

One personal (TypeScript related) counterargument to this: In the foundry-vtt-types, we currently support specifying the data model for actors / items / etc. on a type level. In particular, you can specify the data for each type. We do this in a way that allows type inference based on checking the type of DocumentData. This is extremely useful for TypeScript users. With these changes, we will need to completely redesign how we do this. In the best case, it will be a major rewrite and at worst it's not possible at all.

#

If you want, I can go into more details but it will get fairly specific

unique parcel
#

True. The only way around that, which I currently see is specific
class inheritance for each type, combined with your Proxy solution or my/PF2e's constructor hack.

#

Come to think of it, if we already do such a major overhaul: @mortal tinsel would it be feasible for you guys to allow systems to configure different constructors depending on the actor or item type?

#

(might be a bit off-topic)

wispy ibex
foggy ledge
#

yeah, especially with it being before another D

#

.prepared ?

fossil flame
foggy ledge
#

theDataPreparedByPrepareDataMethodOfTheSystem

stiff cliff
#

Daniel is on to something.

paper pike
#

Seems like the rest of that proposal is pretty agreeable to those of us fixated on the new name. 😛

stiff cliff
#

We really do need to get to the more important topic of property name conflicts. Since that will actually break things.

unique parcel
#

(meanwhile the TS devs start crying in their own channel...)

stiff cliff
#

Unless the lack of conversation on the topic indicates that it's not a very common problem.

stiff cliff
unique parcel
#

I myself have only one property that collides in my system.

unique parcel
bleak patio
unique parcel
#

@mortal tinsel I tested the script you posted and it gave a lot of false positives for colliding properties. Mainly stuff that is core foundry.

bleak patio
#

how could the properties collide? By having properties defined in some Document overrides that are also part of the data?

stiff cliff
paper pike
#

e.g. if Actor.data.data has hitpoints
and in preparation, an attribute on the actor instance is set with hitpoints as the name, this change will cause a collision and probably unexpected results.

unique parcel
#

@bleak patio all documents have a name. My system data also has a name property at the top level.

bleak patio
unique parcel
#

good point

stiff cliff
#

Summary: If the system defines a data property that is named the same as a core data property, the two names conflict. Example: img is a common core data property, if a system has its own img it will now conflict.

paper pike
#

Oh yeah, so this would only really apply to things that are... ^ yep that

unique parcel
stiff cliff
#

Sorry, not a data property. A Class property.

bleak patio
#

basically, it's not related to the stuff inside data, but to properties that we explicitly defined on our classes extending Document

stiff cliff
#

Any package which extends a Document class with their own module or system-specific extension may have defined certain properties on the class which also exist as keys in the data schema. For example, a class like MySystemItem extends Item may have implemented a property like:

unique parcel
#

OK yeah. So I wouldn't have any conflicts at all.

stiff cliff
#

Yeah, I jumbled it up in my head 😆

foggy ledge
#

in some edge-cases a collision is probably still possible, but it seems like you would need to do something relatively weird

unique parcel
#

I mean either it's hard to understand how this could happen or the lack of people talking about already tells you a bunch on how often this occurs.

stiff cliff
#

Anyway, he provided a script to check your system for conflicts, so presumably everyone can just run the script and get hard data

bleak patio
glossy bramble
#

it's certainly a good idea to remove the data.data thing but I hope this doesn't happen in v12 again. this will create a lot of bugs and require a giant amount of modules to be updated. it's everywhere

unique parcel
bleak patio
#

these are conflicts in core

unique parcel
#

Or not, depending on whether you want to see core problems.

mortal tinsel
#

its just got a linebreak it shouldn't have

bleak patio
unique parcel
mortal tinsel
#

if we do this, but the time V10 Prototype 1 is released these core conflicts will be resolved

#

and the remaining list would be non-core conflicts

stiff cliff
bleak patio
#

Just for reference: 0 conflicts in my system (ds4). But I have only created very few properties on the document classes, and those I have created have pretty specific names. So I wasn't expecting any conflicts.

unique parcel
#

I only have the ones in my extended Actor and Item classes and those come from core.

paper pike
stiff cliff
bleak patio
#

So, here is my take on the whole thing:

If we can ensure (with compatibility shims etc.) this doesn't cause too many packages to break in the short to mid term, I am in favour. But we really need to be sure about this. Even if the changes that are necessary aren't all that complicated, it's still a huge amount of work for people who maintain lots of modules, systems, and potentially content. We need to be sure they get enough time to migrate their stuff.

Regarding the concern about separation of concerns of functionality and data: That is a valid concern, but foundry is already mixing this anyways, for example with all the convenience accessors that just forward to data. Even the name Document already sounds like data. So if separation of data and functionality was the goal, I think the battle is lost anyways. So I think it's better to embrace the fact that in foundry, data and functionality are tightly coupled.

One more thing I want to note: In my opinion, this should be a "do it or don't" decision, not a "do it or do it potentially later" decision. While this is a huge change and may result in a lot work for module and system authors, waiting longer just makes it worse. Foundry is a rapidly growing platform, and with time, more and more systems and modules will be created, so the amount of work needed to address this change will just increase in the future. tl;dr: If you want to do it, do it now.

umbral shard
#

@mortal tinsel My main question is, was there a reason that this kind of model wasn't used in the first place?

hardy lance
#

Greets. While the proposed changes doesn't affect my module dev as I use flags for custom data storage I am curious about implementation details insofar as modifications to ClientDocumentMixin are concerned; I assume the proposed changes likely touch this area of code, but perhaps not. I'd like to discuss ideas on how to decouple the App v1 infrastructure from ClientDocumentMixin while maintaining the functionality the current implementation brings and how this can help App v2 and the API in general. I can put together an RFC in that regard.

On the client side as the document model is transformed to be more streamlined from a data access perspective IMHO the extra grafted on functionality / methods to support App v1 gets in the way of this goal.

Whether it makes sense to also evaluate these kinds of changes for v10 when there already will be a fair amount of potential other changes is certainly open to debate.

I'm all for trying to condense as many changes as possible into one release. The ideas I have maintain functionality, cleanup the client side document model, and provide additional flexibility for JS usage / subscriptions to documents / document collections clearing a pathway for App v2 while maintaining the App v1 implementation w/ a few changes.

upbeat ledge
#

You mention that getters will be shimmed to be backwards compatible, but what about calls to update() that then refer to data.x?

mortal tinsel
visual grove
mortal tinsel
bleak patio
mortal tinsel
# hardy lance Greets. While the proposed changes doesn't affect my module dev as I use flags f...

When you mention "App v1" and "App v2" to me that sounds like the terms we use for the Application class which is unrelated to this change and perhaps/probably not what you're referencing?

As for the ClientDocumentMixin is still has an important role to play so that we can reuse the same BaseDocument on both client and server side but assign it scope-specific functionality which is different in each case. I don't anticipate there being significant changes here.

mortal tinsel
bleak patio
#

But I guess I’ll better let Michael speak for himself 😅

hardy lance
# bleak patio It’s definitely about `Application`, but I’m also not sure in what way it is rel...

Tangentially related; also why I tried to just make the post in #dev-support as I know this is a more focused discussion thread. I was told to post here FWIW despite my statement that this is tangentially related to the general cleanup of the document model / purpose of the change being discussed here. As the document model transforms to be more concerned about exposing the data directly then IMHO it may make sense to reduce the other add on methods from ClientDocumentMixin particularly App v1 related like createDialog, deleteDialog, importFromJSONDialog, and the apps tracking object can potentially be handy. I have some ideas in this regard that maintain existing support while actually increasing or generalizing document subscriptions w/ a pure JS API making such functionality useful beyond the GUI layer. I see the current design of ClientDocumentMixin / App v1 coupling as more of a convenience of initial implementation, but a hard coupling to App v1. So again probably wrong forum to discuss this in particular, but work that likely needs to get done to support App v2 as well that is tangentially related to streamlining the document model.

night lintel
#

metadata instead of system? There's a case for it, since it's already being used in Compendium collections.

wooden sparrow
#

as one of the pf2e devs, I think the state following the merge will be better than the one before it, but it's going to take a lot of refactoring. I think we won't have as much problem as the javascript developers who will most likely just be chasing deprecation warnings, but I mostly worry about the "non-code" things where we've stuck paths to things in data objects such as amount of damage weapons do and things like that.

wooden sparrow
night lintel
#

But then... is the metadata in Compendiums really metadata.

keen rain
#

(just string-replace leading "data." parts of path properties)

#

I guess it might be a little tricky to use AEs (or AE-likes in our case) to set flags if they're going to be moved to the document instance level

Screenshot_from_2022-03-15_14-44-58.png
#

or would AE paths also be recontextualized as operating at the instance level?

umbral shard
prisma mantle
#

I welcome this change - thank you for the breakdown. I'm sure that it'll be painful, but perhaps it would be worth having the deprecation warnings for this area specifically be driven by a developer setting somewhere, perhaps even on a per-system and per-module level? This would reduce the amount of noise that one would face when perhaps reworking intra-module/system behavior?

#

Of course, I fully expect this to be enabled globally after a few releases, but it would at least function as a stopgap in the interim.

unique parcel
#

After talking about it with @fathom eagle some, we found that it might be very beneficial for typing efforts if the type property of an item or actor would be in the systemData property (either instead of or in addition to the document). Thoughts? I'm not sure how valid that is outside of typescript.

fathom eagle
#

To explain briefly why it'd help, it's to make systemData a "discriminated union" https://github.com/basarat/typescript-book/blob/master/docs/types/discriminated-unions.md

To explain a bit more thoroughly, currently a very ergonomic part of Typescript is allowing narrowing from one type of actor or item easily. This means something like if (actor.data.type === "specificType") { actor.data.data.uniqueProperty } will infer exactly like expected. (where uniqueProperty may only exist on "specificType" or have differing types but within the if statement it knows its exact type)

To explain why this change breaks this behaviour, the machinery of Typescript doesn't currently allow a "class union" (at least while being extend-able). This means if type is at the top level we can't do the same thing simply rewritten as if (actor.type === "specificType") { actor.system.uniqueProperty }.

bleak patio
# fathom eagle To explain briefly why it'd help, it's to make `systemData` a "discriminated uni...

To expand a bit on this: We currently make this work by making ItemData, ActorData, etc. discriminated unions. As Luke explained above, that actually prevents anybody using the type definitions from extending these classes. For the DocumentData classes that has been an acceptable trade off because extending these hasn't been common until now. But for Document classes, that's obviously not an option. With the changes coming in V10 regarding DocumentData -> DataModel, I suspect that extending DataModel might become much more common anyways, so we will probably run into this problem regardless of weather Document becomes a DataModel or not.

#

Having type available inside system would indeed help a lot in that regard. But of course it's also a bit weird to inject this one property into the data that is otherwise controlled by the system. Might result into conflicts. And outside of TypeScript, there probably isn't any benefit to this.

fathom eagle
#

I think in terms of conflict, type would be a bad property to add yourself. It's true though that I think the only benefit outside of Typescript would be being able to identify the type is given only the system.

Semantically I do think that it makes some sense inside of system because a system defines a set of types that they can be but I do admit the main reason I want it there is for Typescript.

latent falcon
#

On the topic of the RFC, I have particularly repeatedly run into Problems 4 & 5, and in particular the nesting of data . I'm no Javascript expert (correction, I'm no expert, but I know Java and Swift better), and code like this is no doubt necessary in current Foundry, but makes my head spin:

if ( obj instanceof Document ) data = obj.data.toObject();
else if ( obj instanceof DocumentData ) data = obj.toObject();

(mentioned in Problem 4)
So if the proposed change simplifies stuff like this, I'd be happy.

bleak patio
#

but the need for this forwarding highlights the problem

latent falcon
#

One other comment about the ability to set console warning levels; I think I brought this up around the v0.8 data model changes, but a V10_COMPATIBILITY_ERROR setting would be helpful to basically not allow ignoring (or in my case over-looking) deprecation warnings.

#

In other words, with this (debug) setting, your code would always break if you have deprecation warnings buried among 100+ other warnings. I'll see if I can find the suggestion I made previously.

wispy ibex
# fathom eagle I think in terms of conflict, `type` would be a bad property to add yourself. It...

Eh, type can make sense in an item/actor's definition in the context of a system. I can easily think of situations where you would want to store distinct "types" of a given item/actor type where they're mechanically identical other than that certain aspects of the game care about them.

I don't think it's an inherently bad property name to add to a system's data in the context of certain game systems and mechanics.

fathom eagle
#

I think that would be better under a name like subType or something.

#

Or even better some more descriptive term for the system, type is used a fair amount over Foundry so I'd probably think it's document related if I saw it in code.

latent falcon
#

Fundamentally: a mode where a known coming deprecation was promoted to an error rather than just another warning. So a "we're going to remove this in the next version" is an error

mortal tinsel
#

@unique parcel @fathom eagle @bleak patio does the option of having the systemData be an instance of a custom DataModel class potentially help solve this issue with regards to typing?

wooden sparrow
#

how different is the typing in that project than the typing in the pf2e system? We don't seem to have many troubles

mortal tinsel
#

i.e. if item.systemData is an instance of WeaponModel when item.type = "weapon", etc..

bleak patio
fathom eagle
#

That'd work pretty well, I believe. The condition would have to change to item.systemData instanceof WeaponModel--

#

Ah Ghost beat me to it haha

#

but yeah that'd be a way around the issue!

mortal tinsel
#

The current prototype design is that you can configure, for example:

CONFIG.Item.systemDataModels: {
  weapon: WeaponModel
}
#

and your type-specific system data model will be automatically used (client-side only)

bleak patio
fathom eagle
#

I presume big systems like DND5e will move to that as well which would be great especially since it lets you add extra methods to specific items/actors

bleak patio
#

It will still be a bit weird to have to use instanceof for people who are used to just checking type, but I guess we have to make some trade off

mortal tinsel
#

An alternative option is that you can, of course, place type inside your systemData as a document initialization step which would allow you to then typedef the systemData object in the way that you have proposed, for example:

class MyActor extends Actor {
  _initialize() {
    super._initialize();
    this.systemData.type = this.type;
  }
}
somber nexus
#

Would there be any impact to the static Document#create method? I have this in one of my modules and seen it other places before:

const itemData = {
  data: {
    // system data goes here
  },
  img: "some/image.webp",
  name: "The Item name",
  type: "equipment",
};
return await Item.create(itemData);

Would the itemData.data have to be be renamed to itemData.system?

bleak patio
#

it would work if we had everything under our control. but we can just provide types, no runtime implementation

mortal tinsel
fathom eagle
#

Yeah I always thought data.data was a bit awkward and non-specific so I'm definitely glad it's happening

bleak patio
mortal tinsel
somber nexus
bleak patio
sand saffron
#

different classes per item/actor type has been really good to us for pf2e with regards to scaling, so that being encouraged in the core architecture only sounds like a good thing

bleak patio
#

there are quite a few systems that implement this by using either a Proxy, or implementing this by returning individual subclasses from the constructor of a base class

mortal tinsel
#

I could see arguments being made for that still being useful though, but I think the need for it is significantly lessened

green flower
bleak patio
mortal tinsel
#

Returning something other than an instance of the prototype from its constructor is pretty gross

bleak patio
fathom eagle
#

I think the point is that people shouldn't have to do it anymore with the inner data being able to return different classes

sand saffron
#

Its kinda gross in that its hacky, but its one of those hacks where the gains far outweighed the losses, and there really wasn't a way to get that without the silly hackiness

#

so its nice to see it being considered for core without the hackiness

fathom eagle
#

I'd personally be inclined to agree (with Atropos that it's pretty gross) but I've never toyed around with subclasses much myself, building on top of DND5e most of the time. I might not know the advantages of it.

#

yeah it's really a step in the right direction to allow different classes under systemData

sand saffron
#

its because foundry news up the system defined type, which means that only one actor/item instance was supported. To support more, we had to fetch a subclass constructor and call that constructor

fathom eagle
#

Oh absolutely, I mostly mean advantages to how it's done today beyond what allowing the same in systemData (less hackily) allows

#

if there are advantages to today over what's being proposed that might suggest a different design, but I'm currently unaware of any extra advantages currently

upbeat ledge
#

I've run the compatibility test, and it seems to be saying that every single field name in my fcoActor class, which extends Actor, is a breaking change. This would be an issue; surely you wouldn't expect me to use different fields than 'name', 'type' etc. in my actor class?

unknown.png
visual grove
#

I think it was noted that the test does not work right now.

upbeat ledge
#

Oh, I didn't see that.

bleak patio
#

that will go away in V10 because they obviously remove those accessors

#

if you want to check this now, you should search for properties defined by yourself in that list

#

or wait until the first prototype

upbeat ledge
#

I don't believe I define any, to be honest.

bleak patio
#

though pretty unlikely

upbeat ledge
#

The thought of yet another set of changes to the core document model is honestly just depressing and tiring. It will mean eventually refactoring every call to *.data.data or *.data and I don't trust find and replace to do it because I probably use data all over the place as a parameter in functions etc.

#

So I'll have to do them all manually, most likely.

#

1,170 results for data.* and 253 for data.data.*

bleak patio
#

The good thing is, you would be able to take your time with this, due to the long deprecation period

upbeat ledge
#

Assuming the non-breaking changes don't actually break things. Which they did, last time in the change to 0.8

#

Would this need to be changed in the proposed model?

for ( let s of game.data.settings ) {```
#

I think game is a document of some kind, isn't it?

bleak patio
#

from what I understood so far, the changes would have no impact at all on that code. game is an instance of Game, which is simply a class, unrelated to Documents

upbeat ledge
#

Right, but it illustrates my point that the x.data pattern is all over the place for things that aren't documents, making an automated refactoring pretty hard.

visual grove
#

Regex, lots of regex.

upbeat ledge
#

How would regex know the difference between something like game.data (unless I specifically allow for 'game' in the search) vs doc.data.flags?

bleak patio
#

yeah, that's true. Just a simple search and replace probably doesn't cut it

upbeat ledge
#

I think I'd also need to refactor any update call like:

await this.document.update({"data.description.value":DOMPurify.sanitize(event.target.innerHTML)})```
bleak patio
upbeat ledge
#

As I believe that would become an update to description.value

visual grove
#

No.

upbeat ledge
#

Yes, I know, but that just kicks the can down the road for the refactor.

visual grove
#

It will not move, unless the idea of changing the name of the system data happens.

upbeat ledge
#

Unless the deprecation is left in indefinitely.

bleak patio
fathom eagle
#

If you happen to be using something like Typescript or something (which tbf you likely aren't) you can mark data on actors and items as deprecated and search for instances of deprecation, that's going to be my migration path.

visual grove
#

The .data.data change to .data will not alter the data model.

upbeat ledge
#

I am not using Typescript, no.

#

I was referring to this, which says that calls to document.data.foo will be changed to document.foo:


- document.data.data.*
+ document.system.*
- document.data.*
+ document.*```
visual grove
#

If the proposed idea of changing the name of the prop to hold the system data to something like .system or .systemData, then that part of the data model would change and would need to be reflected in the update calls.

#

The old top level was the root of the document.

upbeat ledge
#

It would be weird if the update stayed data.foo when foo is now just a member of document, wouldn't it?

visual grove
#

Not your system data.

bleak patio
bleak patio
#

the paths in update have always been relative to the DocumentData, not the DOcument

upbeat ledge
#

Right, because it's actually a call to update data.data I guess, you just omit the first data. So if the name does change then those will need to be refactored.

visual grove
#

Nope.

#

Full stop.

#

The call directed to the data model, but nothing was transformed.

bleak patio
#

well, if the name of the inner data is changed to system, that will need to change when / if the deprecation period runs out

visual grove
#

Please export one of your own actors and look at the JSON.

#

That JSON is what was getting updated.

upbeat ledge
#

I will likely endeavour to update my system to avoid deprecation warnings as early as possible, because I don't believe in putting stuff off just because it can be, but I don't think that 'you don't have to do this until later' is an argument against how much work is involved, just when the work has to be done.

bleak patio
# visual grove That JSON is what was getting updated.

I think there is a misunderstanding here. The export indeed contains a property called data containing all the system specific data. Other properties are no nested inside that. To update properties inside data, you need to include data in the path to the property.

As the suggestion is to rename this data property to system, the call to update needs to be changed eventually.

visual grove
#

Yea, I'm talking about that.

#

Skimble was walking like if data was dropped that it would reflect on the inner model for updates.

#

doc.update({'data.foo': 2}) becoming doc.update({'foo': 2}), which is not true.

bleak patio
upbeat ledge
#

No, I was saying when you make an update call you're updating data by default, so you omit the first data' in 'data.data'

#

So yeah, any update call I have which is currently data.x would become system.x if the nomenclature was changed to data.system.x

visual grove
bleak patio
#

I think we are all on the same page now, so let's move on 🙂

visual grove
#

Ah, wrong reply.

#

data being renamed to system or systemData was simply a suggestion, prompted by the thought that it would be best to do it now.

#

It's not the crux of it, but would probably be one of the easiest shims to maintain for a long time.

#

Biggest rub would be backend for updates.

bleak patio
#

It is part of the proposal, though, so discussing it is fair.

#

but you are right, it's not the core of it

upbeat ledge
#

The proposal is saying it would HAVE to be changed to maintain backwards compatibility

# 
A significant consequence of this proposal is that we would add backwards compatibility for Document#data so that pre-existing code functions without required changes. Adding this backwards compatibility, however, will require us to change the name used for SystemDataFields inside Actor, Item, Cards, and Card data models. We intend to rename this field from data to system which would - in combination with the other facets of this proposal - change the example usage above to:```
bleak patio
visual grove
#

Eh.

#

Could just also add a shim for .data.data

upbeat ledge
#

Personally I'm not sure how it will help to have people sometimes getting data.system.data instead of data.data.data, but I probably don't understand enough of the core underlying architecture to see the benefits to developers

bleak patio
upbeat ledge
#

I also think system is a bad name in that it implies data relating to the system rather than the system data of a specific actor.

bleak patio
#

at worst it will be data.system, if somebody has a document called data

visual grove
#

The renaming .data to system is not part of the core proposal and was a suggestion.

bleak patio
#

it is part of the proposal

upbeat ledge
#

I thought if you pass a document as 'data' and you want what is currently in data.data, given that the second data is renamed to system, couldn't you still end up with data.system.data?

bleak patio
visual grove
bleak patio
#

the inner data (or system) is available directly on the document

upbeat ledge
#

Oh yeah, gotcha.

bleak patio
# visual grove If you read it, it literally does not mention renaming the inner `data` that is ...

it does:

A significant consequence of this proposal is that we would add backwards compatibility for Document#data so that pre-existing code functions without required changes. Adding this backwards compatibility, however, will require us to change the name used for SystemDataFields inside Actor, Item, Cards, and Card data models. We intend to rename this field from data to system which would - in combination with the other facets of this proposal - change the example usage above to:

- actor.data.data.attributes.strength = 10;
+ actor.system.attributes.strength = 10;

We feel this is a great change because it simplifies the data structure while removing ambiguity about what "data" we are talking about. Because of the backwards-compatible shim that would be added, pre-existing references to actor.data.data would redirect properly to actor.system.

visual grove
#

...I totally missed that that when this first dropped. Hell, I think a number of people did.

#

Or maybe just me.

bleak patio
#

yeah, potentially. It's not very obvious, among the other big changes. Maybe the discussion about the name (system vs systemData) also made it seem like this was a community suggestion

#

but good we are on the same page now 🙂

visual grove
#

Probably.

#

I know I skimmed it while at work, so I know my reading comprehension on it was low.

bleak patio
#

Don't worry, it happens 😉

upbeat ledge
#

That is exactly the same paragraph I quoted 13 minutes ago, mind you

visual grove
#

Code blocks are not the best for reading not-code.

#

That and I still had it in my head nothing in the data storage model was moving.

bleak patio
#

Well, let's not harp on this. The misunderstanding has been cleared up. Let's move on and keep this thread productive 🙂

upbeat ledge
#

Is there an easy way to see if a getter is actually used anywhere? For my extra class I've defined some getters as shorthand ways of accessing parts of item.data.data, like item.data.data.active. As these are defined in the schema I believe I'd have to refactor to use different names. However, searching my codebase it's hard to see whether a call to x.active is using the getter or the inherent property. Or does this change only affect things currently at the item.data level rather than the data.data level, due to the change to accessing core fields at the document level?

#

I'm referring to the breaking change mentioned under the heading [Immediately Breaking] Properties at the document level which collide with model fields

visual grove
#

Override the getter, add logging or a throw and see?

bleak patio
upbeat ledge
#

I was thinking that might be the case, ghost.

bleak patio
#

that section is about properties /functions you might have defined on the document (Actor class etc.), that might be conflicting with properties currently in the outer .data

upbeat ledge
#

Yeah, that makes sense given that it references img. I only do this with stuff in the system data model.

bleak patio
#

unless by chance the outer data has a property with the same name, you shouldn't have any problems then

upbeat ledge
#

Ah, I lie, I have a thing that overrides the visible getter.

bleak patio
#

if you still want to see whether a getter is being used, adding some logging to it like Spice_King suggested is a decent way, I think

upbeat ledge
#

Hmm, that's a tricky one, because I'm overriding the core visible() getter to hide actors that represent objects.

#

I can't change the name of the field there, because then Foundry won't recognise it.

bleak patio
#

I don't think there will be a problem with that particular getter

upbeat ledge
#

No?

bleak patio
#

at least not on actor

#

because it doesn't correspond to a property on the DocumentData

#

or rather ActorData

upbeat ledge
#

Ah yes, I see, wheras img does

bleak patio
#

there is no actor.data.visible, is there?

upbeat ledge
#

No, there isn't, I looked for that.

bleak patio
#

then there shouldn't be any issues with that getter. I think it's being added by ClientDocumentMixin. That won't change, as far as I understood.

upbeat ledge
#

And this doesn't apply to actorSheets, because they aren't part of the document model.

#

(I have a get actorType() method in my sheet class)

#

Alright, well, I'm probably getting far too into the weeds for my own system for this thread, but my overall impression remains that this is going to be a PITA to refactor because I'll basically have to do it line by line, about 600 lines of code, to ensure no find/replace cock-ups. Having said that, for me the biggest issue is that it means I'll probably have to leave users behind again that don't update to 10 quite fast, as I don't really want to gatekeep every single call to data behind a version check.

bleak patio
#

the script that is mentioned in the proposal should give you an overview of all properties that are problematic. Unfortunately, it also lists all of the core properties that currently still are problematic. But if you look through the properties it lists, and none of them is from you, you should be good.

bleak patio
upbeat ledge
#

I suppose it would be possible to write a core getter that returns the correct syntax depending on version and then uses that in string literals wherever the code calls for it.

bleak patio
upbeat ledge
#

Right, but if I update my system to not have deprecation warnings, it becomes incompatible with prior versions because 9 won't have a clue what a call to system.x means (for example).

bleak patio
#

For game systems or modules which extend a Document class, you may elect to suppress these console warnings by setting the LOG_V10_COMPATIBILITY_WARNINGS static attribute of the model to false. This compatibility shim could be modified or adopted by a package developer (without warnings) in order to make the redirection "permanent" without any other code changes required.
You can get rid of the deprecation warnings this way, too.

visual grove
# 
get data() {
  if ( this.constructor.LOG_V10_COMPATIBILITY_WARNINGS ) {
#

Literally provides a way to shut them up until you make the jump over once you stop caring about v9 support.

mortal tinsel
#

I've encountered a troublesome corner of the proof of concept that I'm not sure how to handle from a compatibility/shim perspective.

When a API user makes changes to the system data of an actor, passing the old data. path for system data, i.e.:

actor.update({"data.attributes.strength": 10});

We can migrate that when handling the update to be treated as:

actor.update({"systemData.attributes.strength": 10});

But when the server is done with its work and it returns back to the client the true changes that were applied, that "true change" is to systemData.attributes.strength.

This becomes breaking for Actor subclasses which define Actor#_onUpdate or API users who hook onto updateActor and then inspect the set of changes to test something like:

if ( hasProperty(changes, "data.attributes.strength") ) {
  ...
}

I'm not sure what to make of this yet because its a consequence of the change which, unlike others, is not conveniently shimmed for compatibility.

visual grove
#

Pretend that any change to system.X is also a change to data.x

mortal tinsel
#

You misunderstand, that's the part that is handled - what I'm mentioning above is the server response back to the client.

visual grove
#

And I'd shim it on the receiving side of the client, as to not double send over the wire.

bleak patio
#

Or am I also misunderstanding?

mortal tinsel
#

yeah, its possible, just would require extra shim logic elsewhere in the code. Starts to get a bit less clean around the edges.

visual grove
#

Yea, my thought is that in the client side code, you also expand out system.X changes to also have data.X.

#

I've not ripped into guts of the websocket stuff.

upbeat ledge
#

Could be an issue if it changes the order or number of parameters passed to the hook function, couldn't it?

visual grove
#

Ah, here we are.

mortal tinsel
#

it does not

bleak patio
mortal tinsel
#

the best way to shim this is with Object.defineProperty which does some JS black magic to allow you to add the path to the changes object without it becoming a member of, for example Object.keys() or otherwise being enumerable.

#

but anyways, it's just something that needs to go on the checklist of things

bleak patio
mortal tinsel
#

this is uncomfortable though, because while data -> systemData is the most high profile change and therefore merits this treatment, it's certainly not the only data model change we are making, have made, or will make.

#

to date, we have simply allowed the returned data from the server-side operations to be the true and current data model.

bleak patio
#

I guess this would also have been an issue with the changes to light data in v9?

mortal tinsel
#

my perspective had been sanitize/migrate input - everything else should take care of itself

mortal tinsel
visual grove
#

You could patch ClientDatabaseBackend#_postUpdateDocumentCallbacks as that calls the Hook for updates.

bleak patio
#

I guess people just don't rely that much on the layout of that data in _onUpdate / update hooks...

mortal tinsel
#

but I think hooking onto changes to an AmbientLight and reacting to specific changes is a different and much more niche type of thing than hooking into changes to actor system data.

mortal tinsel
visual grove
#

_preUpdateDocumentArray does the preUpdate hook.

bleak patio
#

Well, it's good we noticed this problem now. It's better to have some time to think about how to deal with this in general, instead of having people scream at you why their hooks are broken. And it's really not limited to this particular change but a problem with changes to the data model in general.

mortal tinsel
#

There's no way we'll catch everything before Prototype 1 is released - which will result in some breakages we did not expect or anticipate.

#

I hope that we'll be able to collect some feedback from developers on what breaks if they run their system through Prototype 1 for a 15 minute test.

#

but our track record on that has not been very good.

bleak patio
#

It also applies to the preUpdate hooks btw. and preCreate, too, I think

upbeat ledge
#

I'll definitely be running Fate Core Official through the prototype so will happily scream incoherently provide feedback if anything breaks before I start making changes.

mortal tinsel
#

we know you'll scream incoherently, you don't have to pretend 😛 ❤️

bleak patio
#

the preUpdateDocument hook also has the change differential data, and people might be relying on certain properties in there

visual grove
#

I won't lie and say that I don't think that this should have been included in the v8 changes. It's going to be a change for the better over all, but it probably would have been nice to have had the pain to refactor it all at once, but I doubt that it would have been thought of.

bleak patio
mortal tinsel
visual grove
#

The fact Foundry is willing to make changes to improve stuff for the better is why I'm here rather than still using Roll20 as a GM, though I've yet to really get a game togeather. Curse of the system dev.

upbeat ledge
#

According to my notes from last time, precreate, create, predelete and delete do not pass a change object in the hook.

visual grove
#

Yea, hindsight is 20/20 after all.

mortal tinsel
#

The only thing that has opened the door for this proposal now is new work on the DataModel and its underlying schema which has generalized away some things that were historically Document-specific behaviors.

upbeat ledge
#
  • Note to self: New standardised hook signatures:
  • preCreate[documentName](document:Document, data:object, options:object, userId:string) {}
  • create[documentName](document:Document, options:object, userId: string) {}
  • preUpdate[documentName](document:Document, change:object, options:object, userId: string) {}
  • update[documentName](document:Document, change:object, options:object, userId: string) {}
  • preDelete[documentName](document:Document, options:object, userId: string) {}
  • delete[documentName](document:Document, options:object, userId: string) {}
mortal tinsel
#

As well as the data object in preCreate and create

upbeat ledge
#

Ah, I thought ghost was saying it would affect the change object in precreate earlier.

mortal tinsel
#

the data object passed to preCreate and create hooks is whatever the user provided initially as the input data for document creation.

visual grove
#

There is no change object for preCreate, just the whole data.

mortal tinsel
#

this input data might contain old data references, rather than new systemData ones

bleak patio
visual grove
#

Could probably use Proxy BS to mirror changes to data into system for the preCreate.

bleak patio
#

preCreate currently has the DocumentData as second parameter, so I guess in the new world, it would just receive the document as the first and the second parameter?

visual grove
#

...Or not, just make what's passed in to the preCreate is not a plain old object, but one with a getter and a setter for data.

upbeat ledge
#

I currently use preCreate for this:

Hooks.on("preCreateActor", (actor, data, options, userId) => {
    if (data.type == "Thing"){```

To prevent creation of the actor if it's of type Thing. I think that would work if the second parameter was also the document, but anyone who's expecting that to be a data model would have to strip a layer of .data from their code, I think.
#

Mind you I could also just refer to the type of the document, I guess.

visual grove
#

Yea, the current nature of the document class has a number of accessors that forwarded to document.data, type is on of them.

#

Going forward, it would stop being an accessor and would be the real value.

#

So, in effect, nothing about .type changes.

bleak patio
#

or... not?

#

sorry, that was incorrect

#

my mistake

visual grove
#

I'm a cranky old 0.5.5 dev, we did not have Documents back then, so I probably still treat them as not existing still.

unique parcel
# mortal tinsel The current prototype design is that you can configure, for example: ```js CONFI...

This is similar to what I was thinking of. Your prototype looks like it would instantiate classes under systemData. Honestly if I'm not overlooking something, I would also be totally fine with core instantiating sub classes of the documents themselves per type. That is what pF2e and my system are currently doing. Both cases you can then check with a simple instanceof, but having the document be a subclass would be even more preferable to me.

bleak patio
unique parcel
bleak patio
#

Ah, yes, i misread your message, sorry. Yeah, this is only about the systemData for now, as Atro explained. See #953299044611031120 message for his take on different Document classes per type

unique parcel
#

I guess I just like to have that behavior on the top class instead of having to reach into the data model first.

bleak patio
mortal tinsel
#

I think I've adequately addressed the issue of shims for pre/post hooks for Document create and update operations.

visual grove
#

Good to hear!

#

Oh, I got a small request, the ability to yank all the shims for testing reasons. Nothing helps you fix is than setting everything on fire.

mortal tinsel
#

not a small request, unfortunately, given the way the shims are integrated into the architecture

visual grove
#

Darm.

mortal tinsel
#

but there should be a good way to cause shims (when accessed) to error rather than warn

wispy ibex
#

Depending on how many shims there are, "just delete them from your local foundry.js and watch the world burn" could possibly be an option for local testing

mortal tinsel
#

which is arguably better than removing them

visual grove
#

Fair.

#

I only really care about supporting two major versions, but having a hard shutoff on the shims would be nice for pre V11.

mortal tinsel
visual grove
#

Kinda also need it on the back end too, if there is shims put in there.

#

Also, might be a thought to namespace the compatibility flag for each shim, if Foundry starts to accumulate more and more.

mortal tinsel
visual grove
#

EX: CONFIG.debug.compatibility.V10_DATA_MOVED_TO_SYSTEM_DATA = CONST.COMPATIBILITY_MODES.FAILURE;

mortal tinsel
#

we've... uh... still got a lot of work to do

unknown.png
#

to be fair it's pretty much entirely dnd5e doing things with .data. which can be suppressed

mortal tinsel
#

Follow-Up Question for the <@&596076164104323104>s

We are discussing this internally and haven't decided our opinion as a team yet, but given that this RFC thread is open I think I'll collect thoughts on this aspect of the refactor as well.

While we are making these reorganizations, do we want to try and address our naming debt that documents which specifically are embedded inside a Scene are currently named as AmbientLightDocument as opposed to simply AmbientLight as all other Document types are named? The reason for this was that in the pre-v8 days of Foundry we had AmbientLight extends PlaceableObject and we didn't want to rename that class for fear of breaking packages on the canvas.

It's hard to say in hindsight whether that decision (to avoid further V8 breakage) was a good one or not. The naming conventions regarding documents embedded inside a Scene are a clear area of technical debt that we have been living with.

In an ideal world (without fear of breaking modules), the document class would be named AmbientLight and its canvas representation would be AmbientLightObject (or something TBD). We need to carefully balance the amount of V10 breakage though, as we are already tipping the scales pretty heavily towards disruption. Thoughts?

visual grove
#

At least it's easy to shim.

#

With a spot of proxy magic, you and even make warnings for touching it.

stiff cliff
#

I don't know that this has sufficient benefit really.

tawny depot
#

Would this come with a similar indefinite depreciation to the document model changes? (I'm generally on team "if it's worth breaking it's worth breaking now", as a big disruptive patch and several not very disruptive patches is probably better than several moderately disruptive patches. Not entirely sold on it being "worth breaking" though)

tawny depot
# stiff cliff I don't know that this has sufficient benefit really.

It's a thing I occasionally trip up on, and sometimes see folks in #macro-polo fall over... But yeah, not sure it's worth doing unless there's some architectural reason to be poking around in that space anyway (like there is for the data.data to system change).

And tbh most of that is just people who were around pre-0.8 using "Token" when they mean "TokenDocument"

wispy ibex
#

Or .entities, I see a chunk of that in there too. It does suck when old code breaks, but it's often for the better overall in the long run

mortal tinsel
#

I am inclined to agree with others that it's probably not worth the added disruption given what we already have on the plate.

#

It's worth considering though, because it's definitely lingering tech debt where documents are concerned

visual grove
#

Ah, I've not poked the guts of the lights, going to guess you saying it's not is due to there being AmbientLight being used already.

stiff cliff
#

On the other hand, I've been working over on The League on a proposal regarding PlaceableObjects.

And if there is a decision to break any eggs, might as well break the whole basket.

mortal tinsel
#

AmbientLight extends PlaceableObject is a piece of code which predated documents so when we added Document class in V8 we could not (as we otherwise would have wanted to) define AmbientLight extends Document and we had to name it AmbientLightDocument instead.

mortal tinsel
visual grove
#

While I agree with the core idea, I have zero skin in that game. I kinda expect that a number of people may get bent out of shape over it being a part of V10.

#

It may be a thought to chose wholly new name for it, then it should be a bit easier to shim.

#

SceneLight, PlaceableLight, LightSource or something of the like. (I'm not looking in the source to see if they are in use or anything.)

wispy ibex
#

Yeah, "ambient" doesn't seem like the ideal word in the first place. It's technically correct verbiage, but most people will initially associate "ambient" with indistinct/general things, rather than point-source lights

visual grove
#

I'd make an argument for the name PointLight, makes room for any other shaped light source.

green flower
#

Personally I'd rather just update things and not worry about what it breaks. If it's worthwhile that the broken things need to be fixed, then they'll be updated. I realize this may be seen as a selfish/naïve approach, but I'd rather things just get overall better than worry about the few things that will get left behind

frank radish
#

Counterpoint: If it deals with AmbientLighting, shouldn't it be called such?

#

Consistency helps learning

visual grove
wispy ibex
# green flower Personally I'd rather just update things and not worry about what it breaks. If ...

There's a balanced middle-ground to be had. There are a ton of modules/systems out there which are maintained by people volunteering their work in their free time. Progress is important, but it's also worth being mindful of people who will be negatively impacted. There are hundreds of thousands or millions of lines of code running through Foundry that have to be maintained by people who have lives to live inbetween Foundry code updates.

visual grove
#

I do know I would like light lines/boxes for sci-fi maps that a point light is not ideal for.

half mica
#

There has already been a lot of discussion about the proposed v10 changes, but I'd still like to voice my opinion:

I fully understand that people are frustrated about core changes like these, but Foundry is at a really early stage right now. 1-2 years after release is nothing for a product that will probably be around for 15-30 years. I welcome any change that helps to put FoundryVTT on a spot where its core API is as clean and precise as possible.

And lets be honest, the data vs data.data thing is something that all of us stumbled over. Its hard to understand, hard to document and lots of systems (especially the ones not using types) are passing around documents and data indiscriminately, so often people guess which they have to use (data or data.data).

#

In the long term and in hindsight, I'm confident that this will be seen as the right step. Yes backwards-compatibility is important, but the API should be as precise as possible. And I'd rather have things break sooner than later.

keen rain
mortal tinsel
#

A PIXI.DisplayObject is more analogous to an Application (in the DOM) than it is to the underlying Document which that application renders UI for.

keen rain
#

yeah, I agree the split is warranted

keen rain
#

Oh, sorry, I see at the end. Swapping names has its own baggage, but I guess the harder question is what to name the placeables

#

There's a future confused #dev-support member wondering what went wrong when they called AmbientLight.toObject() expecting an AmbientLightObject 😁

#

have a style convention to always invoke placeables from a namespace, yielding Placeable.AmbientLight 🤷

mortal tinsel
#

I think, generally, with there being so many different concepts in the Foundry API more namespacing would definitely be a good thing.

#

what movement in that direction, if anything, we'll be able to do in V10 I'm not sure.

bleak patio
#

To be honest, I also don’t see that big of a value in this change. Sure, those names are a technical dept, but compared to other things, it’s pretty small. It’s just names after all, and personally, I have gotten used to the …Document names already by now. Yeah, it might be a bit confusing to new developers why some document classes have Document in the name and some don’t, but I don’t think it’s a biggie.

#

Personally, I don’t really care either way. If that change is made, it’s just some small changes in my system and modules. Not a lot of time needed to fix that. But the benefit also is pretty small, as mentioned above.

#

But for the overall community, it seems like an unnecessary disruption with little benefit

mortal tinsel
upbeat ledge
#

The 23rd of March is the day after my birthday and I have the week off. My system will probably be working fully without deprecation warnings by that evening, heh.

mild current
#

First message here says Prototype 1 drops on your birthday

#

GL issue says 23rd

wispy ibex
#

It's Schrodinger's release date; it's both the 22nd and 23rd 'til it releases on one of them and we find out which one it is

mild current
#

GL issue says the Prototype 1 release will be available on the 23rd, and i guess it technically is if it is released 22nd 😄

#

No biggie, but i suspect 22nd is the correct date here

burnt scarab
#

If the builds have names, this one has to be schrödingers build xD

green flower
solid sun
# mortal tinsel As it relates to this conversation, however, there is an important technical req...

If I had no prior knowledge of the API, I'm fairly certain that I would think that applying is persisted to the database and updating is in memory... It's certainly not obvious on first glance and I think that this will be a point of confusion.
In fact, I just read the RFC on GitLab (I was on a trip yesterday) and I intended to come here to ask why a new word "apply" would be used over "update" which is more clear.

I also think that "data" is a more meaningful term than "system"...

Otherwise, I'm on board with this proposal.

But I resonate with the comments about how it becomes increasingly harder the more packages you have to update. I have close to 20 myself and I am not looking forward to the refactoring. Even pasting your block of code into each package and publishing a release will take me around an hour (not to mention how much time that would be without the auto-publish scripts you have asked us not to use).
I'm not sure if there is a point to my complaint, other than perhaps a request for a package release API on the website.

#

What about info instead of system? It's shorter and it describes what it is more effectively.
(I would think that system would refer to the current game system in use or something along those lines, not storage)

solid sun
fathom eagle
#

honestly I'm happy with systemData. system is more meaningful to me in that data is a very generic term and just system feels too terse. I'd expect item.system to maybe refer to the system that created it not the system's data. sysData works if everyone knows the abbreviations but they don't. I can spare three keypresses; sy[tab]. The direction is goes is very bikesheddy though.

I think the main thing that is confusing here is that apply... well applies the change but update doesn't do the type of update that some people expect, that is persisting it.

mortal tinsel
#

@solid sun I think you make some fair arguments, but the one that I struggle most is the emphasis on a package publication API (relative to its time savings)

not to mention how much time that would be without the auto-publish scripts you have asked us not to use

Let's presume for a moment that your build pipeline does everything that is needed to release a new Foundry module version except update the foundryvtt.com/admin page.

You would then need to go update the admin page for 20 packages which would take you, well, let's say 10 minutes because you're going slowly and you double check your manifest URLs. Everyone's time is valuable, I get it, but 10 minutes is presumably only a small portion of the time you would be spending to update that number of modules in the first place.

I'm in favor of making things faster, but I'm also in favor of actually doing the calculus to identify where marginal improvements in productivity or functionality are most valuable - and this just ain't it.

solid sun
#

Fair enough. I'll reply in #dev-support since it's off-topic here.

solid sun
#

I think a bad pattern is to choose a sub-optimal name when renaming things in order to keep backwards compatibility. It's probably better to just give it the best name and disregard whether that name was previously used for something else.

fathom eagle
#

I mean it's somewhat a bad pattern but sometimes the first name was the best name because old code will still break but worse it won't break noisily. Some of my least favourite things to fix are when it's possible it's an old way of doing things, an interface that another module exposes, or literally just a mistake.

#

Changing names makes the name worse but at least if there's behaviour change people can tell by looking at current docs what it probably should be

#

and it'll break in a fairly obvious way

stiff cliff
solid sun
#

Maybe in-memory modifications could be done with modify?

paper pike
#

This is a problem as old as time, I haven't yet heard a good word for "yes that definitely makes changes but not permanently on the backend" as opposed to "yes that definitely makes changes permanently on the backend" in my career. I expect a word will have to get picked and then documentation will have to suffice. update change modify apply save commit push sync upsert, all have pros and cons.

I believe in the staff to make a good pick and then its just a matter of documenting and educating

wispy ibex
#

I suspect there'll also be a strong tendency to keep the current nomenclature as much as possible. Even if it's not perfect, it's far better to not touch stuff than it is to change it to be slightly more appropriate

stiff cliff
#

Using update for what it is today is good, but the "this makes a change and doesn't save it to the DB" verb... that's tricky.

frank radish
#

pretend

solid sun
#

I like this because it actually does describe what is happening really well and is not as ambiguous as other options

#

I know it's not the most technical wording etc, but it's great

#

@stiff cliff Why don't you like it?

stiff cliff
#

It implies more of a degree of "this isn't being saved" than is actually intended.

#

There are situations perhaps when this is exactly correct.

#

But not all.

burnt scarab
#

I also know the pattern where you can supply a parameter telling the function to just "pretend" / not actually commit. e.g. prepare locally but do not send to master or only when I tell you to

stiff cliff
#

Also, it doesn't imply change of a value.

#

prepare might be a better choice.

#

But isn't logical in a non-DB context

burnt scarab
#

kind of difficult really to find something that makes sense.

stiff cliff
#

Yes

visual grove
#

localUpdate

stiff cliff
#

Someone said update({}, { commit: false }) iirc.

#

Which is much more verbose but much more accurate.

visual grove
#

Eh, that touches the data, should be the options object.

#

Yup.

stiff cliff
#

I prefer the idea that the default assumption is always to mutate the database, and only when giving an explicit and clear "don't do that" would it do otherwise.

Plus, on the previous side-discussion of update vs. apply for data models: It would just be update, and commit: true would simply do nothing extra.

grand charm
#

Generally coming from other languages/frameworks I never understood the need for update. Everywhere else I just modify object itself and the just run save()/persist() to save into database (or even use orm methods data.save(object)

#

I get that there may be some calculations that need to be rerun but in that case setters just should run them

stiff cliff
#

It does seem a little odd that update() doesn't just "save the current state"

wispy ibex
grand charm
#

Sometimes I would really like to just be able to have object and direct database access

solid sun
#

Maybe even just use setters so that an update operation happens every time you modify the object

grand charm
stiff cliff
burnt scarab
wispy ibex
stiff cliff
grand charm
#

I would really want to have stupid simple objects and full control over them, but this is probably another discussion

wispy ibex
solid sun
# stiff cliff They can not be `async`

Yeah. That's indeed the main issue. Maybe setters could be used synchronously instead of apply and then update to asynchronously save the current state to the server?

stiff cliff
frank radish
burnt scarab
#

now also schrödingers save states? 😉

wispy ibex
stiff cliff
visual grove
#

Also, you might not want to save everything.

grand charm
stiff cliff
grand charm
#

It’s commonly used

burnt scarab
#

true...

wispy ibex
#

I suspect this discussion is drifting a ways outside of the intent of this thread at this point. Foundry's fundamental server an data persistence architecture isn't up for discussion, just a little bit about the nomenclature

visual grove
#

Will note that none of this is really on topic.

solid sun
grand charm
#

For me the name persist/save would really mean that we are saving to db

#

Update is more like “update current object, but don’t save”

wispy ibex
grand charm
#

So you run update then persist and you are sure server is in correct state

solid sun
stiff cliff
wispy ibex
visual grove
#

I'll say this, stop bringing up this topic here since it's really far off topic.

#

That or y'all can eat mutes.

stiff cliff
#

I don't think anyone is suggesting that it do so. The discussion about the archetecture mereley informs the question of what to call a given action, and what all the actions really mean.

grand charm
stiff cliff
#

Except that as @solid sun pointed out, apply sounds like "save this" which it specifically is not doing.

solid sun
#

I don't think it's off-topic to discuss the meaning of terms in a thread dedicated to an RFC which is proposing to change the meaning of those terms.

visual grove
#

Changing the semantics of document updates is not part of this RFC, thus it is off topic.

wispy ibex
#

There's also a finite amount of going around in circles on a given topic that's sane in a thread that core devs are trying to catch up on. It's best to avoid them needing to read a dozen pages of comments on the semantics of words and how people would rather things work if they were designing things when the staff are trying to catch up and look for actionable suggestions.

mortal tinsel
#

I've read all the conversation for the past while about update vs apply vs other suggestions. My impression from reading is that there isn't a better proposal, or if there is a better proposal, it hasn't been presented in a way that makes it clear that its better.

#
  1. We have a clear requirement to persist document changes to the database. We have used update for this since the dawn of time as it's the "U" in "CRUD" which is pretty much the most standard way to think about functions for interacting with persistent storage. I cannot imagine us changing this.
  2. We also have a clear use case (although not a requirement) for a method which makes local changes, either as part of a handling a server response or as part of making changes which don't require a persistence operation.

None of the alternatives for how to name thing #2 which have been offhandedly mentioned for this are presented in a way that is clearly a better choice than apply(). If there is an option that is clearly a better choice than apply this would be a great time to identify it.

fathom eagle
#

Yeah, I don't think there's a clear better one here. I'll admit I'm more fond of update with commit: true or not but having update and apply is just fine imo!

#

thanks for the succinct reply

wispy ibex
#

Looking at synonyms, modify, revise, adjust, mutate, or something along those lines could work as a slightly more accurate verb than apply, but I don't think apply is inherently bad either.

I think that whatever term is used, most people will get used to it and adjust to it but nothing will be perfect in everyone's eyes

solid sun
#

commit: false as an option would be great. pretend is also very self-explanatory.

mortal tinsel
#

update({}, {commit: false}) is okay. I would personally prefer to have a different named method.

solid sun
#

Why is that? Are there advantages to a different named method, but with practically the same call signature?

mortal tinsel
#

it's not the same call signature though, not really

#

update(changes, databaseModificationOptions) vs apply(changes, changeApplicationOptions)

#

the keys/values of the options are not really overlapping

#

yes, the changes are maybe the same

somber nexus
#

What's the difference between an apply method and directly updating the document through normal assignments? I guess I don't understand what the use case for a non commit update is.

mortal tinsel
#

I think personally what would be most frustrating about reusing .update is when reading code to have to read carefully to differentiate between local and persisted changes

#

Having separate method names makes it easier for me to quickly read and understand intent

#

My concern there is not speculative. The current state of v9 is that I have to read carefully to see whether it’s a document.data.update() or a document.update()

#

It’s currently a problem

stiff cliff
#

I thought document.data.update wasn't going to exist anymore?

mortal tinsel
#

The functionality needs to exist, it cannot remain at document.data.update and needs a new home

fathom eagle
#

I think in the case that we have multiple updates sitting around with different purposes, yeah apply is probably necessary... I like another parameter though so there might be a different refactoring here? Like changing document.data.update to a completely different name or to require commit as false (I believe it's local only?).

stiff cliff
#

Ah, yeah that would be confusing.

Though I feel like apply in particular implies the wrong thing.

mortal tinsel
#

It quite literally is applying changes to the data model…

stiff cliff
#

I also agree that update is the right verb for the true CRUD operation

paper pike
#

mutate is interesting. It is the closest verb to what is happening locally. But perhaps only to JavaScript devs?

stiff cliff
fathom eagle
#

Yeah I honestly think between update and apply anything else would be prettttty unusual so I'd personally stick with one of the first two.

#

With the data versus document update I'd actually swing a lot closer to requiring apply

heavy trench
#

perhaps unhelpfully

Other than basic semantic arguing for the sake of arguing

Does it really matter what the hell it's called as long as everyone knows what it does and the jsdoc note on it explains it clearly?

mortal tinsel
#

Names are important!

stiff cliff
#

In the same way that data.data is fine, yes.

heavy trench
fathom eagle
#

Haha it's not like names almost ever destroy the capability to work with something but trying to get a semantic consistency does feel important

stiff cliff
#

Especially when you really really don't want to have to change it again later.

solid sun
#

I think that having separate named methods could be confusing for beginners since they'll likely not know about apply immediately (because it's less useful/popular) and look for workarounds...

heavy trench
#

It just feels like, perhaps, spending as many hours as have been spent arguing for or against .apply() might be a bit unnecessary.

#

i feel like at this point it could be dataModel.ephemeralTurkey() and as long as we know that turkey means it's applying changes to the data model without persisting them we'd be good

solid sun
tender merlin
#

Well, let's apply some thought to this and after awhile we can come back and update the thread with a final answer...

Nah, works fine for me 🙂

fathom eagle
#

Anathema is pointing out that apply and update are small points of a big RFC that've gotten a lot of attention

#

and that ofc they want feedback but it's been a bit long of a discussion

heavy trench
tawny depot
solid sun
mortal tinsel
heavy trench
tawny depot
mortal tinsel
#

I’m annoyed that people are going to be annoyed when the method is DataModel#apply

solid sun
#

I don't think it's used very often, so most people will probably just not know about it until after they have been banging their head for a few hours and finally ask on Discord to find out it's a weirdly named method instead of an option for update 😉

stiff cliff
#

I think nearly everyone will just mutate the properties of local objects directly and won't know that apply exists.

heavy trench
#

Don't think it's all that weird for what it does, and the documentation for .update could probably include a "temporary updates to the data model should instead use apply()"

tender merlin
#

I disagree on the "weirdly" named.

The docs will reflect it, viewing the function within the code will show it, and there's a logical thought pattern to apply something temporarily vs updating permanently - especially when the default method for permanently changing a document is, in fact, update.

mortal tinsel
#

We are our own most significant users of the API which matters a lot to us as creators of the software. Apply is used a ton by the core software. I think at the end of the day that this design is probably more for me and for the team than it is for the community of module devs

heavy trench
#

but i'll also say as someone who doesn't code by reading the API or the doc strings (because i'm a terrible coder)- I'm only gonna see a new method if i'm struggling to figure out how to do something on my own, or if i see it in someone else's code and go "The hell does that do?"

#

so i don't think that's any different for .apply or otherwise

mortal tinsel
#

Perhaps the solution is to make it private and just use it internally and let module devs manually update the data state however they want to.

stiff cliff
#

My worry is that the two verbs would be mixed up. Which one updates the DB? Update or apply? It's not that we can't find out, but especially from users of other frameworks, apply sounds like the one that "completes" the update.

tender merlin
#

I mean, the name doesn't matter if you don't know it exists in the first place, no matter what you think it might be. Mutate, apply, alter, what have you. And what might seem the most likely to look for for me might not be the same to others.

But the base API docs will show the method (if it's not private) and be able to be seen.

stiff cliff
heavy trench
#

i think if the documentation is there to indicate the difference between apply and update; logically with no other understanding i'm going to assume update updates the data persistently and i'm gonna go look up what apply does.

bleak patio
heavy trench
#

but to be straight honest no new coder is coming in and going "Oh boy i can't wait to read this entire API documentation."

An overwhelming majority of community devs get their start by looking at what other community devs are doing; the code in their modules and systems

#

so i don't really think it'd be all that much of a risk for confusing newbie coders like my dumb ass

stiff cliff
mortal tinsel
#

Genuine question because I can’t think of one: Is there a prominent JS framework that uses .apply() for the update pillar of persistence?

stiff cliff
#

I don't know one that uses the word apply, but commit and execute are common in the context of prepared database queries and transactions. It's less that it's a verb used in place of update and more that it's a verb that feels like it means "okay now do the thing" and in this context it's the one that doesn't do the thing. Where mutate, change, modify, etc. feel like synonyms for update, apply feels like it means something more.

frank radish
#

UseForNow()

#

Regardless, the docs will teach the newbies.

bleak patio
frank radish
#

So I think this thread has accomplished its task. No blood in the streets over the concept itself, just the word for it

mortal tinsel
#

I'm going to declare a moratorium on this conversation and say that I am entirely unconvinced with the provided arguments against .apply() and so that will be the method name in V10 Prototype 1. If it ends up sucking for reasons we don't anticipate we can revisit the conversation after people have hands on feedback time.

stiff cliff
#

Fair enough

stiff cliff
visual grove
bleak patio
keen rain
#

I look forward to the opportunity to use model.apply.apply

mortal tinsel
visual grove
#

OK, it was not documented on the RFC.

mortal tinsel
#

Don’t have a link, but it’s open on the milestone

visual grove
#

I think in this case, it's shimable.

#

Since if the new apply function gets an Actor instance, it can instance check it and fix it up.

mortal tinsel
#

Eh…. I suppose. That feels a bit ugly and would maybe be best to just be a breaking change. I’ll consider it though

visual grove
#

It is ugly, not going to lie.

#

But it would at least mean that things can bridge both V9 and V10.

bleak patio
#

I know my implementation would break 😄

visual grove
#

Still better than nothing I think.

#

And that's only the most basic of shimming.

#

The question is if it's worth doing.

bleak patio
#

To be honest, I don't think mangling that up together is a good idea. I would much prefer a true breaking change if we need to name the thing apply

visual grove
#

I admit, I have no skin in the game of AEs yet, but I have been wanting to play with them.

bleak patio
#

quitely mentions that there still are other potential names that don't result in conflicts

solid sun
#

Yeah. I don't see why having this name is worth the breaking change...

stiff cliff
#

I would say this completely changes the discussion. Semantics are one thing, but having to rename existing methods for the sake of this name is another story. Especailly when ActiveEffect#apply is itself a very well named method.

Plus... are active effects really going to be limited to Actors only forever? Refactoring this method to be used with other types of document wouldn't break its current contract, just expand it. But changing its name to applyToActor would mean future expansions of the AE system might require multiple nearly identical but differrently named methods rather than one unified method.

Whereas something like change shouldn't cause a conflict, the only instances of it that I can find are the jQuery change method. I can't find any modify or mutate, rolls have alter though I don't think that would be a conflict.

There are a bunch of "just as good" options that won't cause any breaking changes, and also might be less confusing.

bleak patio
#

I had plans to do that eventually in my system