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

[Feature Request] Sort environment variables #1046

Closed
Billy- opened this issue Jul 17, 2018 · 16 comments

Comments

Projects
None yet
3 participants
@Billy-
Copy link

commented Jul 17, 2018

  • Insomnia Version: 5.16.6
  • Operating System:Ubuntu 18.04

Details

The variables in "Manage Environments" are automatically ordered alphabetically. While this is good some of the time, sometimes I'd like to logically group certain variables together and in a certain order.

It would be nice to have this optional - perhaps the alphabetical sorting could be a (default on) option?

@welcome

This comment has been minimized.

Copy link

commented Jul 17, 2018

👋 Thanks for opening your first issue! If you're reporting a 🐞 bug, please make sure
you include steps to reproduce it. If you're requesting a feature 🎁, please provide real
use cases that would benefit. 👪

To help make this a smooth process, please be sure you have first read the
contributing guidelines.

@gschier

This comment has been minimized.

Copy link
Collaborator

commented Jul 17, 2018

Ah yes. The environment is currently saved as a JS object, which doesn't maintain key order. I sort it so at least it comes out in a consistent order.

This will actually be a surprisingly tough one to fix but I agree that order would be nice.

@develohpanda

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

@gschier I'd like to pick this up! 🎊 Do you have an approach in mind, given it may be a tough one to solve?

@gschier

This comment has been minimized.

Copy link
Collaborator

commented Apr 24, 2019

Awesome @develohpanda! This is a tough one though. I have two ideas for how to do this.

  1. Store environments as stringified JSON objects instead of JS Objects. However, we'd need to make sure that this does not affect things like exports, plugins, etc.
  2. Store an additional object that somehow maintains the sort order of the environment (including nested keys).

In my opinion, option 2 is more complicated but has much less possibility of affecting things that may be relying on the existing environment format.

Thoughts?

@develohpanda

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

No worries, up for a challenge and a chance to learn something! Admittedly I have not looked at code yet in this area so will come back to you once I familiarize. In theory I think we are better off with option 2 as well.

Do you have an ideal solution for this? I imagine it would be option 1 considering there is no data duplication. The migration would be tricky though, since on first run with a new solution the data would be stored on users' computers as a JS Object while we expect a JSON string.

@gschier

This comment has been minimized.

Copy link
Collaborator

commented Apr 24, 2019

The migration would be tricky though

Each model actually has a migrate method which is called whenever an object is pulled out of the local database. The migration in that case would be the easy part. Here's an example

export async function migrate(doc: Object) {
doc = await migrateBodyToFileSystem(doc);
doc = await migrateBodyCompression(doc);
return doc;
}

Do you have an ideal solution for this?

After thinking about it, I think the ideal solution would be to keep environments as JS Objects (option 2). If we converted the object to a stringified object then we'd have to constantly be converting them back and forth in order to work with them. This might add additional risk of data loss. For example, if somehow the app accidentally fails to parse/stringify the value for some reason.

Option 2 is also nice because the second object to store the key order will be strictly used for UI purposes only, so it provides a nice separation of concerns.

@gschier

This comment has been minimized.

Copy link
Collaborator

commented Apr 24, 2019

As for the implementation of the second object, my initial thought would be to store the key order under a lookup table containing a full path to each object within the environment (I'm open to suggestions here).

const exampleEnv = {
  "nested1": {
    "nested2": {
      "a": true,
      "b": 10,
    },
  },
  "foo": [1, 2, 3, {"hi": "there"}],
}

const exampleEnvOrder = {
  "$": ["nested1", "foo"],
  "$.nested1.nested2": ["a", "b"],
  "$.foo[3]": ["hi"],
}
@develohpanda

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

Thanks for explaining the migration! That looks very handy. I raised a concern about data loss through serialization initially but edited it out for lack of context of how it currently works 😅 I agree that having to unnecessarily convert back and forth is bound to cause issues which would also be very difficult to debug.

If we really do walk down the path of storing a second object, we could store the same data as a map, because a map keeps its order (ref). I would prefer to use a common pattern and am concerned that re-ordering JSON via a second object would require a lot of custom code.

I'll do some further investigations into each of these methods. :)

@develohpanda

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

I'll do some further investigations into each of these methods.

Going through this a bit more, I'm starting to lean towards your lookup suggestion. I decided to do some experimentation and it seems this method shouldn't be as complex as I first thought (famous last words). Currently I have a small script that generates the lookup table from an input object, and now I'm working to consume that lookup table to generate the desired JSON.

While thinking about it, the difference between using a lookup table vs a map is that the map also stores the value. If we continue to store the environment as a JS Object for processing and a map for UI, then we are storing the same data/value in two different ways which is no better a solution to storing as plain JSON.

@develohpanda

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

I decided to do some experimentation

I've uploaded my results and code into a repository (https://github.com/develohpanda/json-lookup-order) to see if using a lookup is a viable approach we could pursue. It looks promising from the exploratory testing I have done, but naturally would need some thorough unit tests for edge cases.

Let me know what you think 😄

@gschier

This comment has been minimized.

Copy link
Collaborator

commented Apr 25, 2019

Nice, thanks for sharing some code 😄

I took a quick look at your code and it looks great at first glance.

I feel like a good API for this would be:

/* Some psuedo-code definitions */

orderedJSONStringify(obj: Object, orderMap: OrderMap) => string;
orderedJSONParse(jsonString: string): {obj: Object, orderMap: OrderMap};

Calling orderedJSONStringify would stringify an object in the correct order and calling orderedJSONParse would parse a JSON string and also return a second object for the order.

@develohpanda

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

Sounds good! I'll update my repo with your suggestions. I feel like this is generic enough to create as a standalone package and consume in insomnia-app - would you suggest doing it this way or roll it into the monorepo? Keeping it separate would simplify testing for it.

@gschier

This comment has been minimized.

Copy link
Collaborator

commented Apr 25, 2019

I was going to mention that before but forgot. If you'd like to create a package for it that'd be great. I'll leave that choice up to you 🥂

@develohpanda

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

I'll keep it separate and create a package 😄🥂

@develohpanda

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

@gschier update for this - I've created a package with this implementation and will start work on now pulling this into insomnia.

@gschier

This comment has been minimized.

Copy link
Collaborator

commented May 10, 2019

Awesome! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.