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

[TASK] Consider using monaco for expression editor #3137

Closed
igarashitm opened this issue Jul 22, 2021 · 36 comments · Fixed by #3444
Closed

[TASK] Consider using monaco for expression editor #3137

igarashitm opened this issue Jul 22, 2021 · 36 comments · Fixed by #3444

Comments

@igarashitm
Copy link
Member

igarashitm commented Jul 22, 2021

Current expression editor is a quick hack from PoC and won't scale. We should redesign it by probably using existing code editor frontend library like monaco - https://microsoft.github.io/monaco-editor/

We should finish this research before #982, #1139, #2093

Also I expect monaco would work well together with LSP, see #3138 as well.

@pleacu
Copy link
Collaborator

pleacu commented Jul 27, 2021

Hey @igarashitm - I've added "monaco-editor": "^0.26.1" to the atlasmap and atlasmap-standalone package JSON files. It builds fine with tests skipped but when I try to build a storybook I run into jest errors. There must be somewhere I need to add the monaco-editor dependency. Any ideas? Thkx!

 FAIL  src/Views/SourceTargetView.spec.tsx
  Test suite failed to run

    Cannot find module 'monaco-editor' from 'ExpressionContent.tsx'

    However, Jest was able to find:
    	'./ExpressionContent.stories.tsx'
    	'./ExpressionContent.tsx'

    You might want to include a file extension in your import, or update your 'moduleFileExtensions', which is currently ['ts', 'tsx', 'js', 'jsx', 'json', 'node'].

    See https://jestjs.io/docs/en/configuration#modulefileextensions-array-string

    However, Jest was able to find:
    	'./ConditionalExpressionInput.module.css'
    	'./ConditionalExpressionInput.module.css.d.ts'
    	'./ConditionalExpressionInput.tsx'

    You might want to include a file extension in your import, or update your 'moduleFileExtensions', which is currently ['ts', 'tsx', 'js', 'jsx', 'json', 'node'].

    See https://jestjs.io/docs/en/configuration#modulefileextensions-array-string

      36 | import { IExpressionNode } from '@atlasmap/core';
      37 | import { css } from '@patternfly/react-styles';
    > 38 | import { editor } from 'monaco-editor';
         | ^
      39 | import styles from '@patternfly/react-styles/css/components/FormControl/form-control';
      40 |
      41 | let atIndex = -1;

@igarashitm
Copy link
Member Author

@gashcrumb
Copy link
Collaborator

It's a Jest configuration issue as I recall. When going through this exercise for Syndesis I believe this particular block fixed it. This links to the 2.x branch when Monaco was working btw.

@pleacu
Copy link
Collaborator

pleacu commented Jul 27, 2021

Ok - general question - assuming that we're eliminating the one/two line conditional input text box, what should that now look like. We could move the f(x) button up to the main taskbar with the other button icons but what happens when you press it? There's two distinct actions that must be supported:

  • enable/disable the conditional expression
  • edit the conditional expression

We could solve the issue by:

  1. Having two buttons
  • negative: Busies up the main task bar with two more icons
  1. Having one button that pops up a dialog where you can choose to edit or enable/disable
  • negative: increase the number of clicks to do what you want to do

wdyt?

@igarashitm
Copy link
Member Author

I expect to keep current look with one line input box, but the code editor features like syntax highlight and auto completion enabled. We might want to add multi line editor, but it would be an addition, hopefully transparently expand from one line editor with one click.

@igarashitm
Copy link
Member Author

seems like it needs some trick to make it single line - microsoft/monaco-editor#2009

@pleacu
Copy link
Collaborator

pleacu commented Jul 27, 2021

Ah - okay - do we want two lines max?

@igarashitm
Copy link
Member Author

igarashitm commented Jul 27, 2021

I would go with single line until we see a problem, and just one more click expands to full screen editor. I wonder if it's possible to do something similar to the field label we already have, otherwise expression will be way too long. It definitely has some collapsing feature, comment could be collapsed to a single line. Need more research to see if it works inline.
keywords: custom folding provider

@pleacu
Copy link
Collaborator

pleacu commented Aug 5, 2021

Currently with the content-editable DIV/markup we establish separate DOM containers for each section of the edit window and these translate into TextNodes and FieldNodes. The monaco editor uses a single container element but does have text ranges with id. I know when I'm adding text and when I'm adding a field element as text. I would need to create and track the element nodes but I don't want to have to adjust the ranges as text was added/removed - looking for a way monaco can do that for me...
wdyt?

@igarashitm
Copy link
Member Author

I don't have an answer for that, but one thing to note is that we'll soon need real tokenizing for function auto completion. TextNode/FieldNode stuff works only when it's just an alternate of those. The function closing parenthesis and arguments break that assumption I guess.

@pleacu
Copy link
Collaborator

pleacu commented Sep 20, 2021

Hey @igarashitm - kinda stuck - source code here: https://github.com/pleacu/atlasmap/tree/i3137
When I build I get this error:

[INFO] @atlasmap/atlasmap: (typescript) Error: /home/pleacu/git-clone/atlasmap/ui/packages/atlasmap/src/UI/ExpressionContent.tsx(45,10): semantic error TS2305: Module '"../../../../node_modules/@atlasmap/core/dist"' has no exported member 'IExpressionNode'.

Here is dist:

This is what it should look like:

[pleacu@fedora dist]$ find . -name '*.d.ts' | grep 'expression'
./contracts/expression.d.ts
./models/expression.model.d.ts
./services/mapping-expression.service.d.ts

This is what I'm generating:

[pleacu@fedora dist]$ cd /home/pleacu/git-clone/atlasmap/ui/packages/atlasmap-core/dist
[pleacu@fedora dist]$ find . -name '*.d.ts' | grep 'expression'
./atlasmap-core/src/contracts/expression.d.ts
./atlasmap-core/src/models/expression.model.d.ts
./atlasmap-core/src/services/mapping-expression.service.d.ts
[pleacu@fedora dist]$ cat ./atlasmap-core/src/contracts/expression.d.ts
export interface IExpressionModel {
    updateFieldReference(mapping: any, insertPosition?: any): void;
}
export interface IExpressionNode {
    uuid: string;
    str: string;
    getUuid(): string;
    toText(): string;
}

What's odd is that the error will move around. Sometimes its common-util that's missing, sometimes the init services. It's definitely something in my code on this branch. If I build w/ just by modified package.jsons, etc its build fine. Anyway - if you could take a quick peek I'd appreciate it. Very weird....

@igarashitm
Copy link
Member Author

I don't know if this is the direct cause of that, but this causes some problems for sure - pleacu@53144b1#r56799323

@pleacu
Copy link
Collaborator

pleacu commented Sep 20, 2021

Yes - I had to disable the test in places - there's an issue within the monaco editor itself that it was flagging. Thkx for the pre-review.

@pleacu
Copy link
Collaborator

pleacu commented Sep 22, 2021

@igarashitm - different issue - what is the total definitive list of callable Atlasmap functions from within the expression window?
So these should be callable:

sc1

There's also lcase, uppercase - also what about the field actions/transformations (i.e. append, etc). I'll add to the monaco editor menu.

@igarashitm
Copy link
Member Author

It has to be retrieved from backend, but that REST endpoint hasn't been implemented yet - #2093

@pleacu
Copy link
Collaborator

pleacu commented Sep 22, 2021

Right - I was going to set up the plumbing for it - I'll leave it for now.

@igarashitm
Copy link
Member Author

So these are the pure functions
https://github.com/atlasmap/atlasmap/tree/master/lib/core/src/main/java/io/atlasmap/functions

And all the transformations are available including custom one, here are the OOTBs
https://github.com/atlasmap/atlasmap/tree/master/lib/core/src/main/java/io/atlasmap/actions

@pleacu
Copy link
Collaborator

pleacu commented Oct 4, 2021

Okay - been struggling with the expression-based spec tests. I'm now using the monaco monarch tokenizer in the core as well as the UI. I use it to parse/process the expression in serialize/deserialize, The problem is that I can't register the Atlasmap tokenizer in a dummy editor. The monaco imports basically define a namespace that matches the object I get when I render. In the tests I can no longer parse the expressions or add/ remove field tokens from the expr. Unless I can find a way to artificially establish an editor tokenizer object I'll have to remove the expression tests... Any ideas?? Something in Jest?

This is up to date:
https://github.com/pleacu/atlasmap/tree/i3137

So - my issue is really this:
https://github.com/pleacu/atlasmap/blob/53144b17c4cb689348c2ec00e69e24527dcb6fc7/ui/packages/atlasmap-core/src/services/mapping-expression.service.ts#L66

languages is undefined in test but okay everywhere else.

@igarashitm
Copy link
Member Author

I don't have an answer, but I got a bunch of hits by searching with "monaco jest", some old ones say monaco doesn't work in jest, not sure if it's still true... would need a deeper research...

@pleacu
Copy link
Collaborator

pleacu commented Oct 8, 2021

ref: @igarashitm
My best efforts at getting the expression-based spec tests to work haven't succeeded. I can mock them but I need the tokenizer to actually function. Uncomment these two lines:

https://github.com/pleacu/atlasmap/blob/160ca7eec2ad27bfa2c02a3fef9c95afe6ce518b/ui/packages/atlasmap-core/src/utils/mapping-serializer.spec.ts#L392

To see the issue (monaco object doesn't exist in headless Jest env).

Issue 2 is this error:

[INFO] @atlasmap/standalone:   ● Test suite failed to run
[INFO] @atlasmap/standalone:     TypeError: /home/pleacu/git-clone/atlasmap/ui/node_modules/monaco-editor/esm/vs/editor/editor.api.d.ts: Property right of AssignmentExpression expected node to be of a type ["Expression"] but instead got undefined
[INFO] @atlasmap/standalone:       at Object.validate (../../node_modules/@babel/types/lib/definitions/utils.js:130:11)
[INFO] @atlasmap/standalone:       at validateField (../../node_modules/@babel/types/lib/validators/validate.js:24:9)
[INFO] @atlasmap/standalone:       at validate (../../node_modules/@babel/types/lib/validators/validate.js:17:3)

I've found some mention of this 3 years ago. It might be order related. Any ideas would be helpful.

My i3137 branch:
https://github.com/pleacu/atlasmap/tree/i3137

Thanks!

@igarashitm
Copy link
Member Author

igarashitm commented Oct 8, 2021

In order to see if it works in a simple project, I'm trying to embed monaco-editor in an example React app (already ejected, so config/scripts are exposed)
https://github.com/igarashitm/monaco-jest-test

However, it seems the mojority of the users are using https://github.com/react-monaco-editor/react-monaco-editor and not much info available for doing it manually.

@pleacu
Copy link
Collaborator

pleacu commented Oct 11, 2021

Ya - the object loader, reliance on the monaco webpack plugin and how the custom language tokenizer fit in made me decide to use the monaco/ monarch platform directly.

@pleacu
Copy link
Collaborator

pleacu commented Oct 12, 2021

@gashcrumb - this is the monaco issue

@igarashitm
Copy link
Member Author

@pleacu submitted a PR to your branch with Jest setup - pleacu#369
Details about the setup - https://github.com/igarashitm/monaco-jest-test/blob/main/README.md

@pleacu
Copy link
Collaborator

pleacu commented Oct 20, 2021

@igarashitm - Hey Tomo - so what's missing is the monaco editor object so the Position class can be instantiated. Do I need to add something to the mapping-serializer.spec.ts? The expression model will execute this as an example:

this._nodes[idIndex].position = new Position( lineNumber, idTokens[i].offset + 1 );

Here is the error I will see:

atlasmap/core:       at Function.Object.<anonymous>.MappingSerializer.deserializeFieldMapping (src/utils/mapping-serializer.ts:410:37)
@atlasmap/core:       at Function.Object.<anonymous>.MappingSerializer.deserializeMappings (src/utils/mapping-serializer.ts:957:29)
@atlasmap/core:       at Function.Object.<anonymous>.MappingSerializer.deserializeMappingServiceJSON (src/utils/mapping-serializer.ts:325:25)
@atlasmap/core:       at src/utils/mapping-serializer.spec.ts:936:27
@atlasmap/core:   ● MappingSerializer › remove a field node from a conditional expression
@atlasmap/core:     TypeError: monaco_editor_1.Position is not a constructor
@atlasmap/core:       637 |         );
@atlasmap/core:       638 |         let fn = null;
@atlasmap/core:     > 639 |         const fieldPosition: Position = new Position(
@atlasmap/core:           |                                         ^
@atlasmap/core:       640 |           lineNumber,
@atlasmap/core:       641 |           idPosition + 1
@atlasmap/core:       642 |         );
@atlasmap/core:       at ExpressionModel.Object.<anonymous>.ExpressionModel.createNodesFromExpr (src/models/expression.model.ts:639:41)
@atlasmap/core:       at src/models/expression.model.ts:365:29

ref editor.api.d.ts:

export class Position {
    /**
     * line number (starts at 1)
     */
    readonly lineNumber: number;
    /**
     * column (the first character in a line is between column 1 and column 2)
     */
    readonly column: number;
    constructor(lineNumber: number, column: number
...

@igarashitm
Copy link
Member Author

@igarashitm
Copy link
Member Author

I checked out your branch again and uncommented the remove a field node from a conditional expression test then I got

 FAIL  src/utils/mapping-serializer.spec.ts
  ● MappingSerializer › remove a field node from a conditional expression

    expect(received).toBeDefined()

    Received: undefined

      965 |           (n) => n.toText() === `\${/city}`
      966 |         );
    > 967 |         expect(cityField).toBeDefined();
          |                           ^
      968 |
      969 |         // Remove the 'city' field from the expression.
      970 |         mapping?.transition.expression.removeToken(cityField?.getPosition());

      at src/utils/mapping-serializer.spec.ts:967:27

It looks like it went beyond the point it's failing for you (deserializeMappingServiceJSON())?

@pleacu
Copy link
Collaborator

pleacu commented Oct 20, 2021

So - I did the same thing - it fails at the same point - 967 because of a failure at `expression.model.ts:639:

@atlasmap/core: FAIL src/utils/mapping-serializer.spec.ts
@atlasmap/core:   ● MappingSerializer › remove a field node from a conditional expression
@atlasmap/core:     expect(received).toBeDefined()
@atlasmap/core:     Received: undefined
@atlasmap/core:       964 |           (n) => n.toText() === `\${/city}`
@atlasmap/core:       965 |         );
@atlasmap/core:     > 966 |         expect(cityField).toBeDefined();
@atlasmap/core:           |                           ^
@atlasmap/core:       967 |
@atlasmap/core:       968 |         // Remove the 'city' field from the expression.
@atlasmap/core:       969 |         mapping?.transition.expression.removeToken(cityField?.getPosition());
@atlasmap/core:       at src/utils/mapping-serializer.spec.ts:966:27
@atlasmap/core:   ● MappingSerializer › remove a field node from a conditional expression
@atlasmap/core:     TypeError: monaco_editor_1.Position is not a constructor
@atlasmap/core:       637 |         );
@atlasmap/core:       638 |         let fn = null;
@atlasmap/core:     > 639 |         const fieldPosition: Position = new Position(
@atlasmap/core:           |                                         ^
@atlasmap/core:       640 |           lineNumber,
@atlasmap/core:       641 |           idPosition + 1
@atlasmap/core:       642 |         );
@atlasmap/core:       at ExpressionMode

@igarashitm
Copy link
Member Author

It doesn't reproduce for me. Do you have a local change? possibly a duplicate test with a same name remove a field node from a conditional expression?

@pleacu
Copy link
Collaborator

pleacu commented Oct 20, 2021

No - we even have almost the same line numbers. Can you commit your changes to my branch? I'll download exactly what you have and see if I can new Position(1,1). I'm building from the top but I'll also try building from ui:

cd /home/pleacu/git-clone/atlasmap/ui
yarn build

Also - can you build from ui with https://github.com/pleacu/atlasmap/blob/8f587a27afe19a693317d1ef9b8184fbb42508e4/ui/packages/atlasmap-standalone/package.json#L11
in place?

thkx!

@igarashitm
Copy link
Member Author

pleacu#370
You have 2 bullet points under FAIL src/utils/mapping-serializer.spec.ts where I get only one - former is what I see and latter I don't see, the line number is for the former I think. So from that log it looks like a separate error.
That line in atlasmap-standalone package.json looks same.

@pleacu
Copy link
Collaborator

pleacu commented Oct 20, 2021

Okay - I'm getting your results now. My diff seemed almost identical to yours. thkx!

@igarashitm
Copy link
Member Author

Instead I found this

(node:453481) UnhandledPromiseRejectionWarning: TypeError: monaco_editor_1.Position is not a constructor

seems the promise rejection is thrown beyond test case and losing the error info. It might help to catch it and print the error.

@igarashitm
Copy link
Member Author

OK found some bugs in the setup
pleacu#371

@igarashitm
Copy link
Member Author

same setup for standalone
pleacu#372

pleacu pushed a commit to pleacu/atlasmap that referenced this issue Oct 29, 2021
pleacu pushed a commit to pleacu/atlasmap that referenced this issue Nov 1, 2021
Fixes: atlasmap#3137

dont need to update yarn.lock

fix yarn.lock

removed  NODE_OPTIONS=--trace-warnings
pleacu pushed a commit to pleacu/atlasmap that referenced this issue Nov 2, 2021
Left the changes to webpack.config.js commented out as they caused a build-time error.
Build is correct and UI|Expression works okay in storybook.
Fixes: atlasmap#3137
Next 3 to 6 months automation moved this from To do to Done Nov 2, 2021
pleacu pushed a commit that referenced this issue Nov 2, 2021
* fix: Add monaco editor support for conditional expressions.
Fixes: #3137

dont need to update yarn.lock

fix yarn.lock

removed  NODE_OPTIONS=--trace-warnings

* chore: fix jest-esm-transformer path

* Comment updates and fixes storybook.
Left the changes to webpack.config.js commented out as they caused a build-time error.
Build is correct and UI|Expression works okay in storybook.
Fixes: #3137

Co-authored-by: Tomohisa Igarashi <tm.igarashi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging a pull request may close this issue.

3 participants