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

Replaced angular.toJson with Utilities.toJson [#CanConHackathon] #7924

Merged
merged 2 commits into from
Apr 10, 2020
Merged

Replaced angular.toJson with Utilities.toJson [#CanConHackathon] #7924

merged 2 commits into from
Apr 10, 2020

Conversation

Matthew-Wise
Copy link
Contributor

https://trello.com/c/UTjBLFQd/17-replace-instances-of-angulartojson

Decouples angulars toJson to es6 this is a direct port of the angular code to ensure we keep its features. I also ported the unit tests to make make testing easier for everyone.

Matt

@nul800sebastiaan nul800sebastiaan merged commit 76e4b6b into umbraco:v8/contrib Apr 10, 2020
@nul800sebastiaan
Copy link
Member

Great! And the tests work in the DevOps pipeline as well!

image

@bjarnef
Copy link
Contributor

bjarnef commented Apr 27, 2020

@Matthew-Wise do we have an equivalent to angular.fromJson() as well?
It seems this PR replacing angular.fromJson() with JSON.Parse() is causing issues, when inserting an image grid editor in grid content.
#7952

angular.fromJson() is also checking if value is a string:
https://github.com/angular/angular.js/blob/master/src/Angular.js#L1335-L1339

It seems to work when replacing the getter here with the following:

get: function (key) {
return JSON.parse(storage["umb_" + key]);
},

get: function (key) {
      var val = storage["umb_" + key];
      return Utilities.isString(val) ? JSON.parse(val) : val;
}

But I think we should have an Utilities.fromJson as well and use this here instead.

@Matthew-Wise
Copy link
Contributor Author

Jan had already started doing from :) #7952

@bjarnef
Copy link
Contributor

bjarnef commented Apr 27, 2020

@Matthew-Wise the PR #7952 just replaced angular.fromJson() with JSON.parse(). It didn't added a equivalent Utilities.fromJson().

When we have a Utilities.toJson() I think we should have a Utilities.fromJson() as well.

It might work in some cases when wrapped inside try/catch, but here it broke inserting an image grid editor in grid content. https://github.com/umbraco/Umbraco-CMS/pull/7952/files#diff-0f50c5d5842e7132955874d51f872a84L209

I have submitted the following PR which fix this issue: #8014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants