-
Notifications
You must be signed in to change notification settings - Fork 2
Description
Hi,
As it turns out after quite a lot of debugging, using this package broke our code, because it globally changes the way JsonConvert works. The code is in file Html2Pdf.cs (lines 95-99) and Screenshot.cs (lines 100-104), as shown below.
JsonConvert.DefaultSettings = () => new JsonSerializerSettings
{
DefaultValueHandling = DefaultValueHandling.Ignore,
NullValueHandling = NullValueHandling.Ignore
};
The correct way to do this is NOT to change the global default configuration (because it affects every other piece of code in the solution), but to pass this configuration as a parameter when calling JsonConvert.SerializeObject(). In this way, you still have the same level of control, but you DO NOT affect how JsonConvert is used everywhere else. These changes should be done in lines Html2Pdf.cs (lines 106, 125, and 144), and Screenshot.cs (lines 111 and 130). The code shown above, which changes the default settings, should be removed altogether.
In short, changing de default settings of JsonConvert in a package is dangerous, and should be avoided, as it has side-effects in every piece of code outside the package as well, so JsonConvert.DefaultSettings SHOULD NOT be used. An example of this same problem in another package can be seen here:
Azure/azure-functions-signalrservice-extension#229
To solve this problem, we are now taking this code and adding it to our project instead of the package, including the changes that I mention above. That's all for the moment.
Cheers,
Diego