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

Re-Implement property value handling #75

Merged
merged 8 commits into from
Oct 6, 2020

Conversation

jbaiter
Copy link
Contributor

@jbaiter jbaiter commented Sep 29, 2020

This PR re-implements the handling of "property values" to be closer to the IIIF Presentation API spec.
Previously this was implemented in the LanguageMap and Language types, this PR replaces this with a single PropertyValue type.

This is a breaking change, since it changes the public API.
The implementation is backwards-compatible with the old LanguageMap/Language API.

Before/After samples:

// Getting a single value for the label of the manifest in the default language
var old = manifesto.LanguageMap.getValue(manifest.getLabel());
var new = manifest.getLabel().getValue();

// Getting the label of a metadata item in the default language
var metadata = manifest.getMetadata();
var oldLabel = manifesto.LanguageMap.getValue(metadata[0].label);
var newLabel = metadata[0].getLabel();

// Getting multiple values for a metadata item in a specific locale
var oldValues = manifesto.LanguageMap.getValues(metadata[0].value, 'de');
var newValues = metadata[0].getValues('de');

New features of the API:

  • Customizable value joining
// Without a second argument, `getValue` returns the first value in the matching locale.
// If a second argument is present, all values matching the locale will be joined with the argument value.
var concatenatedValues = metadata[0].getValue('de', '<br/>');
  • Passing a list of locales to getValue/getValues:
// If the first argument is a list of locales, Manifesto will interpret it as a list of locale preferences in
// descending order of preference and try to find the value(s) best matching the preference.
var vals = metadata[0].getValues(['de', 'en', 'fr']);

Replace the `LanguageMap` approach with static `getValue`/`getValues`
methods that accept a single locale with `PropertyValue` that offers
member methods that accept zero, one or more locales.
@edsilv
Copy link
Member

edsilv commented Sep 30, 2020

Hi @jbaiter
This looks good. Could we ask to retain the LanguageMap interface for backwards compatibility, to be phased out over time by dependent projects?

@jbaiter
Copy link
Contributor Author

jbaiter commented Sep 30, 2020

Hey Ed, I'd love to, but I'm not sure how I could make the PropertyValue class API-compatible with LanguageMap, given that it's simply an alias for Array<Language> with a few static methods tacked on 🤔

I just tried to make LocalizedValue API-compatible with Language and then make PropertyValue extend Array<LocalizedValue> (which should make it API-compatible with LanguageMap).

This works in principle, but the getValue/getValues methods get erased at runtime, i.e. any method that returns a PropertyValue will effectively only return Array<LocalizedValue>, completely negating the API convenience introduced by the new type :-/

-- update:
Found a way around it, the problem is due to ES5 backwards compatibility, where Proxy is not available and subtyping Array is a lot more complicated. With a few nasty incantations, the test suite now passes. I'll add tests for the LanguageMap/Language backwards-compatibility and then push it.

@jbaiter jbaiter marked this pull request as draft October 1, 2020 09:18
@jbaiter jbaiter marked this pull request as ready for review October 1, 2020 10:22
@jbaiter
Copy link
Contributor Author

jbaiter commented Oct 1, 2020

✔️ It should be backwards-compatible with the old API now, I sprinkled a few asserts on that throughout the test suite.

@edsilv
Copy link
Member

edsilv commented Oct 6, 2020

Awesome. Happy to merge @stephenwf ?

@stephenwf
Copy link
Member

Looks great 👍

@edsilv edsilv merged commit 2bcb71d into IIIF-Commons:master Oct 6, 2020
@morpheus-87 morpheus-87 deleted the propertyvalues branch January 20, 2022 14:51
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