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

refactor: create helper function structuredClone and use *old* way #1258

Merged
merged 1 commit into from
Apr 23, 2024

Conversation

danielpeintner
Copy link
Member

point all functions to it so that in future we can easily update

fixes #1252

point all functions to it so that in future we can easily update
@danielpeintner danielpeintner changed the title refactor: create helper function structuredClone ans use *old* way refactor: create helper function structuredClone and use *old* way Mar 22, 2024
@danielpeintner
Copy link
Member Author

Note: in package td-tools we use JSON.parse(JSON.stringify()) also but I don't think the helper functions belongs to td-tools package.

const copy = JSON.parse(JSON.stringify(thing));

Hence we can just use it in packages that depend on core.

Copy link

codecov bot commented Mar 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.82%. Comparing base (1d2e67e) to head (102ee66).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1258      +/-   ##
==========================================
+ Coverage   77.80%   77.82%   +0.01%     
==========================================
  Files          84       84              
  Lines       17523    17532       +9     
  Branches     1781     1781              
==========================================
+ Hits        13634    13644      +10     
+ Misses       3854     3853       -1     
  Partials       35       35              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@relu91
Copy link
Member

relu91 commented Mar 22, 2024

I'm not sure that we should add this if we are not 100% sure that this is a bug of the current supported Nodejs versions. From the issue is not clear if the submitter is just using and old version or not.

@danielpeintner
Copy link
Member Author

I just feel it is not worth to keep the built-in structuredClone if it causes issues. After it was first introduced in #1105 we had to partly revert it in #1152 because of issues. Now we see new issues popping up.

Hence I think it is an improvement until it is better supported. Moreover, with this change we can easily update the code once we feel comfortable.
Anyhow, if others think we should not make the change it is fine by me also.

@egekorkan egekorkan mentioned this pull request Apr 19, 2024
Copy link
Member

@relu91 relu91 left a comment

Choose a reason for hiding this comment

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

As pointed into the commiter call I'm ok merging this PR, it's just refectoring the code to have a single point where to update. We can refactor later if needed.

@relu91 relu91 merged commit dbb75d5 into eclipse-thingweb:master Apr 23, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Error when exposing a Thing because "structuredClone" is not defined
2 participants