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

Remove StringObjectDictionaryConverter and update JintJavaScriptEvaluator #4856

Merged
merged 3 commits into from
Jan 31, 2024

Conversation

sfmskywalker
Copy link
Member

The StringObjectDictionaryConverter class was removed as it is no longer necessary. Instead, the JintJavaScriptEvaluator now wraps objects in ObjectWrapper instances and sets their prototype to Array.prototype if they are array-like. This allows for more convenient and intuitive use of Lists and arrays in JavaScript code within Elsa. A new integration test was created to validate this functionality.

…ator

The StringObjectDictionaryConverter class was removed as it is no longer necessary. Instead, the JintJavaScriptEvaluator now wraps objects in ObjectWrapper instances and sets their prototype to Array.prototype if they are array-like. This allows for more convenient and intuitive use of Lists and arrays in JavaScript code within Elsa. A new integration test was created to validate this functionality.
A new unit test class is added in the IntegrationTests project. The class, located in the JavaScriptListAndArray directory, focuses on testing the engine's ability to handle JavaScript list and array-like objects.
@sfmskywalker sfmskywalker added elsa 3 This issue is specific to Elsa 3 enhancement New feature or request labels Jan 31, 2024
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to test whether this works as expected with arrays, objects and primitives

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. What additional scenarios do you have in mind, specifically? The current test covers dealing with Lists as Arrays.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nested Lists, primitives and objects as the root object and maybe a test with an array instead of a list, just to see that this functionality doesn't somehow break that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, like that. Yes, makes sense 👍🏻

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a few more cases that mixes primitives, objects and regular array: #4857

Do let me know if you see an opportunity for adding more cases.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that with those cases you've covered most of the scenarios. Maybe some edge cases we're still missing, but it is good enough for now.

A new ObjectArrayHelper class has been added into the Elsa.JavaScript module to improve array detection. The previous IsArrayLike method used in JintJavaScriptEvaluator and integration tests has been replaced by the DetermineIfObjectIsArrayLikeClrCollection method from this helper. These changes also provoked an update of the Jint package version from 3.0.0-beta-2057 to 3.0.0.
@sfmskywalker sfmskywalker merged commit a53c792 into v3.0.5 Jan 31, 2024
1 check passed
@sfmskywalker sfmskywalker deleted the issue(4849) branch January 31, 2024 20:11
@sfmskywalker sfmskywalker added this to the Elsa 3.1 milestone Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
elsa 3 This issue is specific to Elsa 3 enhancement New feature or request
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

None yet

2 participants