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

Validation of "format" expression fails when options are provided #8271

Closed
jseppi opened this issue May 21, 2019 · 5 comments · Fixed by #8339
Closed

Validation of "format" expression fails when options are provided #8271

jseppi opened this issue May 21, 2019 · 5 comments · Fixed by #8339
Assignees
Labels

Comments

@jseppi
Copy link
Contributor

jseppi commented May 21, 2019

mapbox-gl-js-style-spec version: 13.7.0 (also tested against versions from 13.1.0 onward)

Steps to Trigger Behavior

  1. Clone this repo, cd to src/style-spec/, run npm install and npm run build

  2. Use the gl-style-validate to validate a style that has a text-filed "format" expression, such as the one in test/integration/render-tests/text-field/formatted-text-color/style.json:
    ./bin/gl-style-validate ../../test/integration/render-tests/text-field/formatted-text-color/style.json

  3. Observe an unexpected validation error:
    layers[0].layout.text-field[1]: Bare objects invalid. Use ["literal", {...}] instead.

Expected Behavior

Running gl-style-validate on a style that contains a text-field "format" expression containing a valid options object should pass.

Actual Behavior

Running gl-style-validate on a style that contains a text-field "format" expression containing results in an error message: layers[0].layout.text-field[1]: Bare objects invalid. Use ["literal", {...}] instead.

jamesseppi at C02W42ZZHV2Q in ~/CODE/mapbox-gl-js/src/style-spec (release-liquid) 
$ ./bin/gl-style-validate ../../test/integration/render-tests/text-field/formatted-text-color/style.json 
../../test/integration/render-tests/text-field/formatted-text-color/style.json:40: layers[0].layout.text-field[1]: Bare objects invalid. Use ["literal", {...}] instead.

If the options are instead changed to be empty objects, then the validation succeeds. For example, if the linked test fixture is modified to have the following "format" expression, then the validation succeeds:

"text-field": ["format",
    ["get", "name_en"], [{}],
    "Italy", { } ,
    "\n", {},
    ["get", "name"], {},
    "Italia", {}
]

cc @asheemmamoowala @tristen

@jseppi
Copy link
Contributor Author

jseppi commented May 22, 2019

I started a branch with a currently-failing test fixture: 8271-format-options-validation, though I have not been successful in figuring out how to fix the bug.

@ahk
Copy link
Contributor

ahk commented May 29, 2019

@jseppi Hi, I'm super new and I tried to follow all the associated library version upgrades, but I don't have enough context to understand all the teams and software components that needed updates or were updated. I can dig in on this, it will just take me a few tries and some time (and gaining a bunch of context that I need eventually anyway).

Is it correct that the format expression bug itself is fixed in patches we already have for mapbox-gl-style-spec? If so, are you waiting on gl-js to integrate something so you can resolve this?

@jseppi
Copy link
Contributor Author

jseppi commented May 29, 2019

@ahk Apologies for any confusion on my part, and thanks for taking a look!

Is it correct that the format expression bug itself is fixed in patches we already have for mapbox-gl-style-spec?

No, that is the bug that this issue is reporting. The format expression does not properly validate with the gl-style-validate tool from the current version (13.7.0) of mapbox-gl-style-spec.

That's what it looked like in your comment.

The expressions mentioned in that comment (text-radial-offset, text-variable-anchor, and text-justify: "anchor") work and are validated correctly. format is the one that does not get properly validated.

@ahk ahk self-assigned this May 29, 2019
@jseppi
Copy link
Contributor Author

jseppi commented Jun 7, 2019

Hey there @ahk - checking on any progress here

@ahk
Copy link
Contributor

ahk commented Jun 11, 2019

Hi @jseppi, I got some help from @asheemmamoowala and found the root cause for this issue. It's all about the Javascript quirk that Number(...) has a different runtime reflection type than new Number(...) I'm preparing a PR and it should be ready for review by tomorrow. I think it's possible the behavior I found creates other validation bugs so I want to just finish checking on that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants