Skip to content

Feature request: UI for passing bindings object #9

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

Closed
sarathkcm opened this issue Apr 6, 2020 · 19 comments
Closed

Feature request: UI for passing bindings object #9

sarathkcm opened this issue Apr 6, 2020 · 19 comments

Comments

@sarathkcm
Copy link
Contributor

I'm loving this library ever since I discovered it. Kudos to the developers.

Often, I spin up a JSONata exerciser tab to debug issues in my expressions, and most of the times I find the need to pass bindings object with multiple properties, and I end up declaring the variables individually in the JSONata expression itself. It would be nice and cool to have a UI to input the bindings as well.

@sarathkcm
Copy link
Contributor Author

Something like this: https://sarathkcm.github.io/jsonata-exerciser/ - I can't get the syntax highlighting to work though

@andrew-coleman
Copy link
Member

This is really nice, I like it.

I think as it stands in your demo, it might confuse users who are not familiar with the ability to pass in external bindings - I wonder if it could be hidden by default, with an option to reveal it for advanced users (my UI skills are not that great).

Also this feature would be really useful if there was a way to import external libraries and bind those to JSONata variables. Since it all runs in the client browser, this might mean automatically inserting script tags to jsdelivr or something. Happy to receive a PR if you can figure this out! 😄

@sarathkcm
Copy link
Contributor Author

I wonder if it could be hidden by default, with an option to reveal it for advanced users (my UI skills are not that great).

Mine neither, but can you please have a look now? https://sarathkcm.github.io/jsonata-exerciser/ - just a simple non-animated toggle.

if there was a way to import external libraries and bind those to JSONata variables

I too think it'll be useful, I use moment js in the expressions and aren't testable in the exerciser.

Adding script tags is probably not the way as global variables won't go away even if the tags are removed. It works for loading JSONata module because its always needed.

I will try and give it a go this weekend.

@sarathkcm
Copy link
Contributor Author

I will try and give it a go this weekend.

I'm really sorry for not keeping up with this promise for months.

I have added the ability to load external libraries from a CDN url and have fixed the syntax highlighting.

@andrew-coleman Could you please give it a try here https://sarathkcm.github.io/jsonata-exerciser/.

If you are happy with the UI and functionality I will raise a PR after a little cleanup of the code (mostly just moving inline styles to css file).

image

@sarathkcm
Copy link
Contributor Author

@andrew-coleman I also would like to confirm a behaviour with Jsonata.

The imported modules are made available in the bindings pane and also passed to the Jsonata expression as bindings. However in Jsonata expression, only the top level module is accessible and works as expected, but if I try to access a nested function it returns undefined.

For example in this screenshot the moment library is available as $moment and works. but $moment.utc is undefined. If I console log the bindings, assign moment to global temp variable in console and use temp1.utc it is available (just to make sure library is correctly parsed and passed to the bindings).

image

image


Is this something related to the way Jsonata handles bindings?

@sarathkcm
Copy link
Contributor Author

I raised a PR #12 with the necessary changes.
Support for saving & loading bindings+external libraries is also added - however, I could only test with mock data as the backend API doesn't support the fields.

@andrew-coleman
Copy link
Member

Hey @sarathkcm this is awesome. The PR looks good to me. I'm going to ask @mattbaileyuk to review it as well (thanks Matt).
In the meantime, I'll try and figure out why functions embedded within object bindings are not working.

@mattbaileyuk
Copy link
Member

mattbaileyuk commented Oct 7, 2020

@sarathkcm As mentioned in the PR review, this is excellent, thank you 👍 I have merged the change into the master branch.

@andrew-coleman Does this now require you to do a redeployment so it goes live on try.jsonata.org?

@andrew-coleman
Copy link
Member

@mattbaileyuk It does, but I'm going to test locally first

@andrew-coleman
Copy link
Member

Hi @sarathkcm Thanks again, this is a great enhancement. Just noticed a couple of issues while playing with this:

  • The expand/collapse works as expected, however, if you try to resize the expanded panel then it doesn't track the mouse as expected. Also, after resizing, if you try to collapse the bindings panel again, it just leaves it filled in with the background colour.
  • The default (invoice) sample has bindings defined (I will change this and add a new sample, btw). When I switch to another sample with no bindings defined (which we will need to support for all saved expressions), the contents of the bindings panel continues to show the previous bindings.
  • When we save/share an expression that contains bindings to external libraries, we'll need to figure out how to serialize that definition so it gets saved and loaded correctly.

Do you think you could take a look at these please?
Many thanks!

@sarathkcm
Copy link
Contributor Author

@andrew-coleman Some comments and queries regarding these issues. Apologies if a bit long.

  • For the first issue

    • I will have to forgo the animation of opening and closing the panel. I tried hooking into SplitPanel resize events to remove the animation only during resize, but it didn't work. Would this be fine?
    • If someone resizes the panel, and then collapses it, and expands it back, do we need to remember the last resize position? Asking because it feels unnecessary complication - even though it would be a better UI experience.
  • The second issue,

    • since we don't have bindings defined for other samples, in setState({ sample.json, sample.jsonata, sample.bindings}) bindings in the state doesn't get updated. This can be fixed by falling back to an empty string when bindings field in sample is undefined, but I left it as is because it's an existing issue with json and jsonata expressions as well. If any of this field is not defined in the new sample, the previous value doesn't get overridden. However, it should work for saved expressions already because there'll be a bindings property in the saved expression with at least an empty string as value.

    • We can fix the issue by adding bindings: "" in all the samples (for consistency with other fields' behaviour).
      If we want to fix it by falling back to an empty string, we should do that for json and jsonata fields as well. What do you recommend?

  • I also noticed just now that external libraries don't get reset when changing samples. Its again the same scenario as bindings, except when changing samples I'm not reading sample.externalLibs. Saving/Re-loading is already handled and will work as long as API accepts and returns the field. How do you want to handle this? Always reset when changing samples or treat them as global like how it is now? It's also possible to define external libs in samples and load all libs while switching.

  • For the third issue,

    • Its already been handled. I'm just serializing the list of libraries, URLs and the variable name, and fetching the library content from all URLs again while re-loading. However, the backend seems to be ignoring the new properties, therefore I could only test by mocking the responses(through a proxy).

Please let me know your thoughts on these, I will give it a go during the weekend.

@andrew-coleman
Copy link
Member

  • I'm happy to keep it simple, not worry about animation and not worry about remembering the size.
  • Let's just set the bindings property of all the samples to the empty string. Or perhaps the empty object with some help text:
{
  // name: value pair(s)
}

Either way I don't mind, we can add to this later.

On the question of global libraries, I think it's fine to keep them global as you've done. I notice that when adding an external library that doesn't exist, it silently fails (apart from logging the 404 error to the browser console). It might be useful to pop up an error box for that. Also, when reloading a saved expression that has an external library that has since gone away will also cause an error. We'll need to think about how to handle that. Let's not worry now though.

@sarathkcm
Copy link
Contributor Author

I notice that when adding an external library that doesn't exist, it silently fails (apart from logging the 404 error to the browser console). It might be useful to pop up an error box for that. Also, when reloading a saved expression that has an external library that has since gone away will cause an error. We'll need to think about how to handle that. Let's not worry now though.

Hm. I followed what's done already when fetching of saved expressions fail. It's logging only to the developer's console, which I think is fine - after all this is a tool targetted to developers.

I will anyways add a very simple self disappearing error message when loading of libraries entered manually fail. Will leave the errors when re-loading saved expressions as it is now - only on the console.

image

@sarathkcm
Copy link
Contributor Author

sarathkcm commented Oct 9, 2020

Raised PR #14 for the fixes/changes.

@andrew-coleman If you could modify the backend API to support new fields, it would help in testing and identifying any issues in save/load.

@andrew-coleman
Copy link
Member

I've fixed up the save/load backend. It all seems to work fine.
It's now live on https://try.jsonata.org/
Thanks!

@sarathkcm
Copy link
Contributor Author

@andrew-coleman Should this be raised as an issue in jsonata repo? I had observed the same behaviour when I do expression.registerFunction as well.

@andrew-coleman I also would like to confirm a behaviour with Jsonata.

The imported modules are made available in the bindings pane and also passed to the Jsonata expression as bindings. However in Jsonata expression, only the top level module is accessible and works as expected, but if I try to access a nested function it returns undefined.

For example in this screenshot the moment library is available as $moment and works. but $moment.utc is undefined. If I console log the bindings, assign moment to global temp variable in console and use temp1.utc it is available (just to make sure library is correctly parsed and passed to the bindings).

image

image

Is this something related to the way Jsonata handles bindings?

@andrew-coleman
Copy link
Member

It's probably because moment identifies itself as a function, so jsonata only allows operations that can be done on functions (i.e. invoke, partial apply, chain).
If you add the following on your bindings panel:

{
   utc: moment.utc
}

Then the expression $utc() seems to work ok. But I guess you knew that.

If it's an object that contains functions, then that works ok. E.g. for the bindings:

{
  math: Math
}

Then the expression $math.cos($math.PI) does the right thing.

@andrew-coleman
Copy link
Member

$moment().utc() also works

@sarathkcm
Copy link
Contributor Author

so jsonata only allows operations that can be done on functions (i.e. invoke, partial apply, chain

Ok, so this is the reason. It's wrong to expect Javascript behaviour in JSONATA. That clears it up.

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

No branches or pull requests

3 participants