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.

Sign up for GitHub

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

Jump to bottom

Add number support for identifiers and LayoutOptions values #260

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add number support for identifiers and LayoutOptions values #260

wants to merge 6 commits into from

Conversation

Daniel-Khodabakhsh
Copy link

@Daniel-Khodabakhsh Daniel-Khodabakhsh commented Jan 30, 2024 •
edited
Loading

This update introduces 3 changes to elk-api.d.ts.:

1. Support integer identifiers

Identifiers in ELK JSON can be a string or an integer according to the documentation.

The id can be a string or an integer.

This is further backed up by org.eclipse.elk.graph.json.JsonAdapter.asId supporting both string and integer JSON types.

    def Object asId(JsonElement id) {
        switch id {
            JsonPrimitive case id.isString: return id.asString
            JsonPrimitive case id.isNumber && isInt(id.asDouble): return id.asDouble.intValue
            default: throw formatError("Id must be a string or an integer: '"   id   "'.")
        }   
    }

With this PR, a new type ElkIdentifier was added which allows you to assign integers as identifiers in typescript without errors:

elk
  .layout({
    id: 'root',
    layoutOptions: {},
    children: [
      { id: 1 },
      { id: 2 },
      { id: 3 },
      { id: 4 }
    ],
    edges: [
      { id: 'e1', sources: [1], targets: [2] },
      { id: 'e2', sources: [1], targets: [3] }
    ]
  })
  .then((data) => /* ... */);

2. Allow number values for LayoutOptions

Layout options can be a number type such as an integer or double. For example, org.eclipse.elk.layered.spacing.nodeNodeBetweenLayers uses double values as mentioned in the documentation.

With the change in this PR, we can now assign numbers without typescript throwing an error:

const elk = new Elk({
  defaultLayoutOptions: {
    'elk.algorithm': 'layered',
    'elk.direction': 'DOWN',
    'layered.spacing.nodeNodeBetweenLayers': 40
  }
});

3. Missing semicolons in the definitions of the interfaces

Although there is no up-to-date specification for typescript, the majority of examples shown in the official documentation have semicolons. There is also inconsistent use of semicolons in this file so this fixes that as well.

@Daniel-Khodabakhsh
Introduce and use ElkIdentifier.
4dc0961 
This change fixes a few issues with `elk-api.d.ts.`:

1. Identifiers in ELK JSON can be a string or an integer [according to the documentation](https://eclipse.dev/elk/documentation/tooldevelopers/graphdatastructure/jsonformat.html#nodes-ports-labels-edges-and-edge-sections).

> The id can be a string or an integer.

Because of this, a new `type ElkIdentifier` was added and used where necessary.

2. Missing semicolons in the definitions of the interfaces. Although there is no up-to-date specification for typescript, the majority of examples shown in the official documentation have semicolons. There is also inconsistent use of semicolons in this file so this fixes that as well.
@Daniel-Khodabakhsh
Support number LayoutOptions values
7b133b7 
Update LayoutOptions so that values such as:

```
'layered.spacing.nodeNodeBetweenLayers': 40
```

is supported.
@Daniel-Khodabakhsh Daniel-Khodabakhsh changed the title Introduce and use ElkIdentifier. Add number support for identifiers and LayoutOptions values Jan 31, 2024
@soerendomroes
Copy link
Member

This looks good. I just need to find time to check whether this effects the conversion between json and other formats (which might take a while since I am quite busy at the moment).

@Daniel-Khodabakhsh
Copy link
Author

Daniel-Khodabakhsh commented Jan 31, 2024 •
edited
Loading

Thanks @soerendomroes !

Concerning the effects on conversion, does this really need to be checked? The reason I ask is because there is no conversion API in elkjs. This PR does not add new functionality, it only fixes an issue in the typescript definitions to prevent erroneous linting for developers.

I did a quick check on the hosted version of elk-live and it also accepts numbered identifiers and layout option values without issues:

elk-live json

however, the conversion page is indeed having issues:

image

Despite the conversion error, this should not block this PR for the following reasons:

  • elkjs has no conversion API
  • elkjs has no elkt support
  • The conversion error above is a bug in elk-live, elk, or sprotty, not elkjs
    • Did a quick check and conversion on elk-live does not use elkjs, it instead hits the elk-live /conversion resource which in turns appears to contact an instance of elk or sprotty hosted on a jetty server
  • elk-live supports numbers as identifiers and layout option values as shown in the first screenshot above (despite the incorrect linting errors in the monaco editor)

Additionally, I just added a new file to test that numeric identifiers and layout option values work. Can you trigger the tests please?

@rienheuver
Copy link

Would love for this to be merged! Using elkjs with Typescript as well and am now also running into point 2 as described above (using numeric values for layout options). So this would be nice to prevent erroneous typing errors

@Daniel-Khodabakhsh
Copy link
Author

Daniel-Khodabakhsh commented May 13, 2024 •
edited
Loading

Were you able to find time @soerendomroes? Did you read my arguments as to why I don't think it's necessary to test conversion for this PR?

@soerendomroes
Copy link
Member

@Daniel-Khodabakhsh Not yet, since we would like to be compatible with ELK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Daniel-Khodabakhsh @soerendomroes @rienheuver