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

ZCLI's zcli.config.apps.json parameters doesn't stringifies objects like ZAT did #184

Open
joelhellman opened this issue May 27, 2023 · 2 comments
Labels
backlog Has been added to the development team's backlog bug Something isn't working

Comments

@joelhellman
Copy link
Contributor

Expectations

I have app parameter setting of type multiline in manifest.js that holds some app settings stored locally as nested JSON.

Now, ZCLI removed settings.json and introduced parameters in zcli.config.apps.json.

When I had this kind of manifest file in ZAT

"parameters": [{"name": "jsonfield", "type": "multiline"}]

and this kind of settings.json file in ZAT: {"jsonfield":{"meaningOfLife": 42}}

and I ran this code:

const client = ZAFClient.init();
client.on('app.registered', function (appRegisteredData) {
    const { metadata } = appRegisteredData;
    const jsonfieldType = typeof metadata.settings.jsonfield;
    console.log(jsonfieldType);
})

the console.log returned "string"

ie. the zat server maybe mapped "jsonfield" in the settings.json to the manifest's multiline field, noticed it was a string (multiline is text) and stringified the value. If one logs the metadata.settings objects, I get

?zat=true
image

Btw, this way of loading settings from app.registered seems okay to me, I've used it in many apps, it's also officially endorsed in docs:
https://developer.zendesk.com/documentation/apps/build-an-app/using-react-in-a-support-app/

To the issue

I expected this zcli.config.apps.json content: {"parameters": {"jsonfield": {"meaningOfLife": 42}}} run by zcli apps:server:

const client = ZAFClient.init();
client.on('app.registered', function (appRegisteredData) {
    const { metadata } = appRegisteredData;
    const jsonfieldType = typeof metadata.settings.jsonfield;
    console.log(jsonfieldType);
})

to also console log "string"

Reality

If I have zcli.config.apps.json content: {"parameters": {"jsonfield": {"meaningOfLife": 42}}} and run zcli apps:server it doesn't returns string but 'object'

Logging the metadata.settings object, I can see the whole parameter JSON root has now already been parsed as a full document, instead of applying the business logic that fields cannot store native JSON in Zendesk v2 apps (AFAIK).

?zcli_apps=true
image

ZAT knew that app settings fields couldn't store JSON, so it stringified the JSON if it noticed an object. ZCLI doesn't account for this.

Another thing is that issue #14 introduced that parameters in zcli.config.apps.json are auto-sent into to the app installation settings when using apps:update. However that will also send the parameter as an object, which Zendesk's ruby backend will then try to serialize.

In the my example, the apps installation setting - which was sent zcli apps:update from parameters in zcli.config.apps.json without any stringification - now landed like this in the production installation app setting as:

"settings": {
   ...
   "jsonfield": "{\"meaningOfLife\"=>42}",
},

and for me, this caused the deployment step of zcli apps:update to stall indefinetly at the deploy step (w/o any error message or timeout)

$ zcli apps:update dist
Uploading app... Uploaded
Deploying app... ⣯    <--- spinned forever

My workaround

So now for my code not to break when I use parameters in zcli.config.apps.json, I had to change my initialization to something like this

const client = ZAFClient.init();
client.on('app.registered', function (appRegisteredData) {
    const { metadata } = appRegisteredData;
    const jsonfieldType = typeof metadata.settings.jsonfield;
    let jsonFieldTypeObject;
    if (jsonfieldType === 'string') {
       try {
            jsonFieldTypeObject  = JSON.parse(metadata.settings.jsonfield);
       } catch (e) {
         // handle error
       }
    } else {
       jsonFieldTypeObject  = metadata.settings.jsonfield;
    } 
})

Which complicates things for me because the app settings now behaves differently once deployed from when I test locally.

Another approach would be to not accept nested JSON in parameters in zcli.apps.config.json. I.e. require each parameter settings there to already be a stringified JSON. I feel that would result in a bad local DX, because it's hard to write stringified JSON by hand.

Fix

Maybe this is too late to fix, since it might mess with existing ZCLI apps, but otherwise I would like if we could have back the original behavior from ZAT, that the zat server considered that app installations cannot have nested objects, so they are JSON stringified by zcli apps:server when running locally with parameters in zcli.config.apps.json the file.

(For the payload sent on update in zcli apps:update in #14, that behavior should follow, altough the current behavior there seems weird to me, as I reported in #183)

Steps to Reproduce

  1. clone https://github.com/joelhellman/zendesk-zat-vs-zcli-settings-repro
  2. zcli apps:server
  3. check output with zcli_apps=true - it will be "object", check dev console
  4. zat server -c settings.json
  5. check output with zat=true - it will be "string", check dev console

Issue details

  • Command: zat apps:server
  • Version: @zendesk/zcli/1.0.0-beta.33 wsl-x64 node-v18.13.0
  • OS: Unbuntu22/WSL2
@joelhellman joelhellman changed the title ZCLI's zcli.config.apps.json parameters doesn't stringified nested objects like ZAT did ZCLI's zcli.config.apps.json parameters doesn't stringifies nested objects like ZAT did May 27, 2023
@joelhellman joelhellman changed the title ZCLI's zcli.config.apps.json parameters doesn't stringifies nested objects like ZAT did ZCLI's zcli.config.apps.json parameters doesn't stringifies objects like ZAT did May 27, 2023
@zach-anthony
Copy link
Contributor

Hi Joel, thanks for the detailed description of the issue. I agree that the typeof operator should not be returning Object when testing your app with ZCLI as this would not be possible, once the app has been installed in your instance. Having reviewed usage from other developers, it appears that the pattern of using nested objects or arrays in 'multiline' parameters is quite common.

While I think that this signals that we ought to have a more suitable types for parameters, I think it makes sense for us to support the same behaviour as ZAT in terms of stringifying objects and arrays (perhaps with a warning or info statement that lets the developer know this will happen)

I can't commit to when we'll be able to release this feature, but have added this to our internal backlog and will keep this issue open until resolved

@zach-anthony zach-anthony added bug Something isn't working backlog Has been added to the development team's backlog labels Jun 26, 2023
@schiguoi
Copy link

+1 on fixing this, I spent far too long trying to figure out why this worked differently from zcli.apps.config.json and post-install using the same settings. Thanks @joelhellman for posting your solution, it was helpful in working around this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Has been added to the development team's backlog bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants