Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Montage data converters#1 #2010

Merged
merged 72 commits into from
Aug 11, 2019
Merged

Conversation

marchant
Copy link
Member

To merge after /feature/npm3

  • Adds a new new converter to map embedded types
  • fixes a deserialization bug
  • adds resolveObjectForTypeRawData method to RawDataService

Copy link
Collaborator

@cdebost cdebost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to be linted.

Looks like some commits got added twice, this PR should be rebased to remove duplicates.

I'll let @tejaede review the data/ related changes.

core/serialization/deserializer/montage-reviver.js Outdated Show resolved Hide resolved
data/service/data-service.js Show resolved Hide resolved
data/converter/expression-data-mapping-converter.js Outdated Show resolved Hide resolved
data/service/raw-data-service.js Outdated Show resolved Hide resolved
data/converter/raw-embedded-value-to-object-converter.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@tejaede tejaede left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we find a more descriptive name than than ExpressionDataMappingConverter? The name says the converter can be used in ExpressionDataMapping objects, but that is true for any object that implements the converter interface so it doesn't tell us much.

The class and its subclasses do something very specific -- use rawData of type Foo to fetch (or create) cooked objects of type Bar where Bar is a level 1 montage-data type -- so we should find a name that reflects that, IMO.

Some options:

  1. RawValueToObjectConverter -- The obvious parent class name for the two subclasses (RawEmbeddedValueToObjectConverter and RawPropertyValueToObjectConverter), both of which are well named, IMO.
  2. ForeignKeyConverter -- the object (and its subclasses) are responsible for most, if not all, foreign keys so this would be apt as well. This would break the association of parent class name to child class name that we would get with # 1.
  3. RawValueToForeignKeyConverter -- combination of # 1 and # 2.

@marchant
Copy link
Member Author

Agreed, how about parent: RawValueToObjectConverter
then RawEmbeddedValueToObjectConverter and 1. RawForeignValueToObjectConverter ?

@marchant
Copy link
Member Author

  • refactor/add convertDataQuery and revertDataQuery to adds more transparency and allow upper layers to expose that query.

cdebost and others added 12 commits February 22, 2019 16:13
If they can't be loaded from the app package, they will be retried under
mr (npm 2 / legacy bundling support).
I suspect this code only exists for historical reasons, but it seems
totally unnecessary, as bluebird will already be cached in the registry
and will not be loaded twice. The hardcoded bluebird path causes an
extra 404 when loading the app with npm 3+.
it is deserialized as the root of a serilization
- adds a future-proof way to get a type's prototype  & constructor
@marchant
Copy link
Member Author

marchant commented Apr 1, 2019

This merged the latest in #master to get sync capabilities in the deserializer, and it needs montagejs/mr#146 to work, which is itself additional to the npm3 work, so both PR go hand in hand

@marchant
Copy link
Member Author

marchant commented Apr 1, 2019

montage tests pass, also tested with a montage data improved version of shop-demo (which wasn't working without these changes as it's the first use of a main.datareel folder that contains all data related assets) as well as Popcorn master. Please review again.

@marchant
Copy link
Member Author

marchant commented Apr 1, 2019

Some test fails as it needs montagejs/mr#146, I didn't change package.json, so it still points to "18.0.0-rc1" where montagejs/mr#146 needs to be merged on

@@ -60,6 +60,9 @@ var ModuleObjectDescriptor = exports.ModuleObjectDescriptor = ObjectDescriptor.s
this.super(serializer);
this._setPropertyWithDefaults(serializer, "module", this.module);
this._setPropertyWithDefaults(serializer, "exportName", this.exportName);
if(this.object) {
this._setPropertyWithDefaults(serializer, "object", this.object);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marchant Is there a spec for serializing/deserializing ModuleObjectDescriptor#object?

@tejaede
Copy link
Collaborator

tejaede commented Apr 10, 2019

An error with DataService deserialization bubbled up some inconsistencies.

  1. DataService._childServices is designed to be a Set, per the documentation. However, DataService.deserializeSelf set the value to an array.

  2. DataService.deserializedFromSerialization called DataService.addChildServices with DataService._childServices. As mentioned above, DataService._childServices is an Array if the serialization includes child services, but is a Set if the value is allowed to lazy load.

I am unable to write to marchant/montagejs so I pushed fixes to my own repo for these issues for your review: tejaede@9829912

The fixes are:

  1. Ensure DataService.childServices is always a set
  2. Use iterator in DataService.addChildServices so the argument can be either a Set or an Array

cc: @marchant

…s objectDescriptorModule as a dependency. This could be made more general but slower for all {“%”: “module_id”} construction in a serialization
Makes sure all child services are added and only once.
Adds ability to deserialize synchronously objetc descriptor references:
- in expression-data-mapping.js
- in raw-value-to-object-converter.js
@marchant
Copy link
Member Author

There's a problem running karma on travis, we need to fix this independently of this PR, merging.

@marchant marchant merged commit 0246d8a into montagejs:master Aug 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants