-
Notifications
You must be signed in to change notification settings - Fork 30
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
feat(xml-support): implement parsing and serialisation from/to Camel XML IO files #2009
base: main
Are you sure you want to change the base?
Conversation
c8dd60b
to
5f8bb32
Compare
@@ -101,7 +102,7 @@ export const SourceCode: FunctionComponent<SourceCodeProps> = (props) => { | |||
code={props.code} | |||
onCodeChange={props.onCodeChange} | |||
customControls={customControls} | |||
language={Language.yaml} | |||
language={isXML(props.code) ? Language.xml : Language.yaml} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[curious-question] Do we already know this information from the resource's serializer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. Serializers are considered at the resource level when CamelResources are being created, which happens in the useEntities hook
const [isOpen, setIsOpen] = useState(false); | ||
const [selected, setSelected] = useState<'XML' | 'YAML'>(camelResource.getSerializer().getLabel() as 'XML' | 'YAML'); | ||
|
||
function onSelect(serializer: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to use function
here? Couldn't we use an arrow function like the other components?
export const SerializerSelector: FunctionComponent = () => { | ||
const { camelResource, updateSourceCodeFromEntities } = useContext(EntitiesContext)!; | ||
const [isOpen, setIsOpen] = useState(false); | ||
const [selected, setSelected] = useState<'XML' | 'YAML'>(camelResource.getSerializer().getLabel() as 'XML' | 'YAML'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems we're leaking this casting in many different places. What do you think starting from the very root? Meaning that at the top-most interface, we define a DSL
property of type XML | YAML
, and from that point, the serializer would have a method that returns one value statically. From there, we propagate that information up to this [selected, setSelected]
, and we won't need the <'XML' | 'YAML'>
part as the type will be inferred from the serializer method.
Continuing on that path, I would leave it up to the camel resource how to instantiate the serializer, meaning that in the onSelect
callback we would pass the serializer string and then the camel resource would change the serializer if finds it, otherwise it could throw an error.
In an ideal scenario, we would like to create new objects to the camel resource.
onSelect={(_event, value) => onSelect(value as string)} | ||
onOpenChange={(isOpen) => setIsOpen(isOpen)} | ||
toggle={toggle} | ||
style={{ width: '20rem' }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we please move this to a dedicated SCSS file?
onSelect={(_event, value) => onSelect(value as string)} | ||
onOpenChange={(isOpen) => setIsOpen(isOpen)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the onSelect
attribute expects a void
callback, but instead of providing one, we're passing a function that returns the output of the onSelect
callback, could we write it in a void
fashion, so it doesn't get decorated with return
?
This:
() => doSomething();
get's transpiled to:
() => {
return doSomething();
}
So for functions that don't need to return anything, is better to write them accordingly
onSelect={(_event, value) => onSelect(value as string)} | |
onOpenChange={(isOpen) => setIsOpen(isOpen)} | |
onSelect={(_event, value) => { | |
onSelect(value as string) } | |
} | |
onOpenChange={(isOpen) => { | |
setIsOpen(isOpen) } | |
} |
@@ -0,0 +1,95 @@ | |||
/* | |||
* Copyright (C) 2023 Red Hat, Inc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Copyright (C) 2023 Red Hat, Inc. | |
* Copyright (C) 2025 Red Hat, Inc. |
* limitations under the License. | ||
*/ | ||
|
||
import { describe } from 'node:test'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems suspicious 👀 , we're using jest
, so this might break in older nodejs versions.
import { describe } from 'node:test';
}); | ||
|
||
it('should parse rest verbs correctly', () => { | ||
const xml = ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible to use an XML
file instead? Even if it's not valid due to not having the <camel>
tag, at least we would get a bit of autocompletion and coloring 😄 .
Coloring is important 😇
splitEntity, | ||
throttleEntity, | ||
} from '../../../stubs/eip-entity-snippets'; | ||
describe('parser basics', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
describe('parser basics', () => { | |
describe('parser basics', () => { |
export type EntityDefinition = | ||
| CamelRouteVisualEntity | ||
| CamelErrorHandlerVisualEntity | ||
| CamelRestVisualEntity | ||
| BeansEntity | ||
| CamelInterceptFromVisualEntity | ||
| CamelInterceptSendToEndpointVisualEntity | ||
| CamelInterceptVisualEntity | ||
| CamelRouteConfigurationVisualEntity | ||
| CamelOnCompletionVisualEntity | ||
| CamelOnExceptionVisualEntity | ||
| CamelRestConfigurationVisualEntity; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it might be better to move this type somewhere else, maybe a bit higher in the structure, as this might come handy for other code.
7aabd4a
to
279cbdf
Compare
…urce files. (cherry picked from commit 3eb48b9)
…ntities to XML (cherry picked from commit f2618a4)
… with Kaoto (cherry picked from commit 7b03dd8)
…format (cherry picked from commit 8971aa5)
…YAML ones (cherry picked from commit 327b7c4)
(cherry picked from commit 5f8bb32)
(cherry picked from commit 37bd939)
(cherry picked from commit 279cbdf)
862fe59
to
8849b16
Compare
…ar and add it as a additionalControls component fix:add missing key to SerializerSelector
|
src/serializers/xml
Fixed issues: