You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by purplecabbage <gi...@git.apache.org> on 2015/04/03 10:26:57 UTC
[GitHub] cordova-wp8 pull request: Added Newtonsoft json dll
GitHub user purplecabbage opened a pull request:
https://github.com/apache/cordova-wp8/pull/79
Added Newtonsoft json dll
Included license files, and meta.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/purplecabbage/cordova-wp8 NewtonsoftJson-DLL
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-wp8/pull/79.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #79
----
commit 418e51ea4f6c6cb644f18f0660efdd8b058644a5
Author: Brett Rudd <br...@gmail.com>
Date: 2015-04-02T00:00:20Z
Move to Newstonsoft.JSON for JSON serialisation
commit b7c17b2f60ccc63f30f84645ba882996c69f4793
Author: Jesse MacFadyen <pu...@gmail.com>
Date: 2015-04-03T08:10:28Z
add meta
commit aed59ff8b18a93db9a20cb937615f23bbd070169
Author: Jesse MacFadyen <pu...@gmail.com>
Date: 2015-04-03T08:23:14Z
Add Newtonsoft license files
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-wp8 pull request: Added Newtonsoft json dll
Posted by alsorokin <gi...@git.apache.org>.
Github user alsorokin commented on the pull request:
https://github.com/apache/cordova-wp8/pull/79#issuecomment-99542404
@purplecabbage @goya could you please look at these failures?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-wp8 pull request: Added Newtonsoft json dll
Posted by purplecabbage <gi...@git.apache.org>.
Github user purplecabbage commented on the pull request:
https://github.com/apache/cordova-wp8/pull/79#issuecomment-99345293
We need to release this first, and file-transfer plugin should declare the required version.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-wp8 pull request: Added Newtonsoft json dll
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on the pull request:
https://github.com/apache/cordova-wp8/pull/79#issuecomment-99336921
So the https://github.com/apache/cordova-plugin-file-transfer/pull/72 probably should be closed then...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-wp8 pull request: Added Newtonsoft json dll
Posted by brodybits <gi...@git.apache.org>.
Github user brodybits commented on the pull request:
https://github.com/apache/cordova-wp8/pull/79#issuecomment-99931329
The Newtonsoft JSON library does seem much easier to use and perhaps more efficient as well, but unlike the .NET JSON `DataContractJsonSerializer`, Newtonsoft does not support serialization of arbitrary types. This becomes a problem with the cordova-file-plugin when it attempts to send a PluginResult with a byte array (`byte[]`) in response to a `readAsArrayBuffer` request. This is the cause of CB-8968, and it could affect other plugins as well.
I have identified two alternative solutions to fix CB-8968:
I. Fix `File.cs` in cordova-plugin-file to send the `PluginResult` with a normal integer array:
```diff
--- a/src/wp/File.cs
+++ b/src/wp/File.cs
@@ -695,7 +695,10 @@ public void readAsArrayBuffer(string options)
buffer = readFileBytes(filePath, startPos, endPos, isoFile);
}
- DispatchCommandResult(new PluginResult(PluginResult.Status.OK, buffer), callbackId);
+ int[] bufferAsIntArray = new int[buffer.Length];
+ for (int i=0; i<buffer.Length; ++i) bufferAsIntArray[i] = buffer[i];
+
+ DispatchCommandResult(new PluginResult(PluginResult.Status.OK, bufferAsIntArray), callbackId);
}
catch (Exception ex)
{
```
Idea thanks to: http://stackoverflow.com/questions/15226921/how-to-serialize-byte-as-simple-json-array-and-not-as-base64-in-json-net
II. In `template/cordovalib/JSON/JsonHelper.cs` add a new function such as `public static string SerializeAny(object obj)` with what used to be the contents of `JSON.JsonHelper.Serialize`, and call this function instead of `JsonConvert.SerializeObject` in `PluginResult.cs`.
I am not sure which solution would have better performance. Solution II only goes through the data once, but solution II delegates the looping to the framework libraries, which *could* do the hard work in C/C++/assembly.
I am happy to issue a PR for either solution. Solution I would be easiest.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-wp8 pull request: Added Newtonsoft json dll
Posted by nikhilkh <gi...@git.apache.org>.
Github user nikhilkh commented on the pull request:
https://github.com/apache/cordova-wp8/pull/79#issuecomment-99129350
@purplecabbage - When do you plan to merge this. There is another related issue now causing WP8 mobilespec tests to crash because this has not been fixed within the platform: https://github.com/apache/cordova-plugin-file-transfer/pull/82
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
Re: [GitHub] cordova-wp8 pull request: Added Newtonsoft json dll
Posted by Jesse <pu...@gmail.com>.
That sounds like a very reasonable approach.
So we'll revert the changes to JSONHelper.cs + leave NewtonsoftJSON lib in
and plugin developers can use it if they prefer.
Sound good?
Yes, currently PG-Build supports only cordova-wp8.
Windows Universal (8.1) is in the works.
@purplecabbage
risingj.com
On Tue, May 12, 2015 at 12:31 PM, brodybits <gi...@git.apache.org> wrote:
> Github user brodybits commented on the pull request:
>
> https://github.com/apache/cordova-wp8/pull/79#issuecomment-101394601
>
> > If we can avoid the breaking change it would be better and in this
> case we have a solution that works well to avoid it
> (apache/cordova-plugin-file-transfer#72). I understand Newtonsoft JSON is a
> better API and is more efficient.
>
> The only real trouble spot I have seen is in the PluginResult class,
> in case a plugin returns an arbitrary data type (such as a byte array). To
> solve this, we can modify the PluginResult to use an internal function that
> uses `DataContractJsonSerializer` (along with a `StreamReader`) to convert
> the plugin result to JSON. I am happy to issue a PR for this if you need it.
>
> Of course, to be absolutely safe, we could revert the changes to
> `template/cordovalib/JSON/JsonHelper.cs` but keep the Newtonsoft JSON
> library for those plugins that want to use it. IMHO, it should be safe for
> this project to use the Newtonsoft JsonConvert functions in all cases
> except for PluginResult.cs. And finally document it for the plugin authors.
>
> An extreme solution would be to rename (or remove) the functions in
> JsonHelper.cs and change the order of the parameters in the PluginResult()
> constructor, in order to *force* the plugin authors to review and update
> their code.
>
> > I would not expect cordova-wp8 to wither and die anytime soon.
>
> In addition, it is my understanding is that cordova-wp8 is (still)
> needed to support Windows Phone in PhoneGap build.
>
>
> ---
> If your project is set up for it, you can reply to this email and have your
> reply appear on GitHub as well. If your project does not have this feature
> enabled and wishes so, or if the feature is enabled but not working, please
> contact infrastructure at infrastructure@apache.org or file a JIRA ticket
> with INFRA.
> ---
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
> For additional commands, e-mail: dev-help@cordova.apache.org
>
>
[GitHub] cordova-wp8 pull request: Added Newtonsoft json dll
Posted by brodybits <gi...@git.apache.org>.
Github user brodybits commented on the pull request:
https://github.com/apache/cordova-wp8/pull/79#issuecomment-101394601
> If we can avoid the breaking change it would be better and in this case we have a solution that works well to avoid it (apache/cordova-plugin-file-transfer#72). I understand Newtonsoft JSON is a better API and is more efficient.
The only real trouble spot I have seen is in the PluginResult class, in case a plugin returns an arbitrary data type (such as a byte array). To solve this, we can modify the PluginResult to use an internal function that uses `DataContractJsonSerializer` (along with a `StreamReader`) to convert the plugin result to JSON. I am happy to issue a PR for this if you need it.
Of course, to be absolutely safe, we could revert the changes to `template/cordovalib/JSON/JsonHelper.cs` but keep the Newtonsoft JSON library for those plugins that want to use it. IMHO, it should be safe for this project to use the Newtonsoft JsonConvert functions in all cases except for PluginResult.cs. And finally document it for the plugin authors.
An extreme solution would be to rename (or remove) the functions in JsonHelper.cs and change the order of the parameters in the PluginResult() constructor, in order to *force* the plugin authors to review and update their code.
> I would not expect cordova-wp8 to wither and die anytime soon.
In addition, it is my understanding is that cordova-wp8 is (still) needed to support Windows Phone in PhoneGap build.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-wp8 pull request: Added Newtonsoft json dll
Posted by brodybits <gi...@git.apache.org>.
Github user brodybits commented on the pull request:
https://github.com/apache/cordova-wp8/pull/79#issuecomment-99960110
Issued PR to fix CB-8968 (automatic build failed, don't think it is my fault)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-wp8 pull request: Added Newtonsoft json dll
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on the pull request:
https://github.com/apache/cordova-wp8/pull/79#issuecomment-99734573
@brodybits follow these steps:
* clone https://github.com/apache/cordova-plugin-file-transfer.git somewhere on your disk
* Create a new application
* Add three plugins:
```
cordova plugin add path-to-cloned-file-transfer-plugin
cordova plugin add path-to-cloned-file-transfer-plugin/tests
cordova plugin add https://github.com/apache/cordova-plugin-test-framework.git
```
* Update config.xml at the root of your app - change `<content src="index.html">` to `<content src="cdvtests/index.html">`
* Run application
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-wp8 pull request: Added Newtonsoft json dll
Posted by purplecabbage <gi...@git.apache.org>.
Github user purplecabbage commented on the pull request:
https://github.com/apache/cordova-wp8/pull/79#issuecomment-99934209
I think we should go with solution 1.
```
int[] bytesAsIntArray = Array.ConvertAll(buffer, c => (int)c);
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-wp8 pull request: Added Newtonsoft json dll
Posted by nikhilkh <gi...@git.apache.org>.
Github user nikhilkh commented on the pull request:
https://github.com/apache/cordova-wp8/pull/79#issuecomment-101378234
@purplecabbage I'm really concerned with this switch for WP8 as it is breaking in nature. It changes how the Cordova WP8 JSON APIs behave and will cause a fair number of plugins to break. All plugins that break will have to make updates to support the new WP8 platform, and hence can't be used with the older version (3.x) of the WP8 platform.
I would propose we don't make this disruptive change for WP8 which is a platform with limited runway as WP8.1+ should be using the Windows platform. WP8 should ideally be on life support and not making disruptive breaking changes that affect the plugin ecosystem.
We should consider an incremental fix for file transfer and other plugins that need better support for JSON parsing instead of changing the platform itself: https://github.com/apache/cordova-plugin-file-transfer/pull/72
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-wp8 pull request: Added Newtonsoft json dll
Posted by brodybits <gi...@git.apache.org>.
Github user brodybits commented on the pull request:
https://github.com/apache/cordova-wp8/pull/79#issuecomment-99371506
> Yeah @brodybits, will have another look at
apache/cordova-plugin-file-transfer#54
:+1: thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-wp8 pull request: Added Newtonsoft json dll
Posted by purplecabbage <gi...@git.apache.org>.
Github user purplecabbage commented on the pull request:
https://github.com/apache/cordova-wp8/pull/79#issuecomment-101383321
yes, it is a breaking change. That is why it is a 4.0.0 change. Plugin
authors need to use engine tags, and we need to make sure they work.
Personally, if I was writing a WP8.1 app, I would use cordova-wp8 and
upgrade stuff. Having a C# wrapper around a html/js presentation will
still be appealing to many. I would not expect cordova-wp8 to wither and
die anytime soon.
@purplecabbage
risingj.com
On Tue, May 12, 2015 at 11:33 AM, Nikhil Khandelwal <
notifications@github.com> wrote:
> @purplecabbage <https://github.com/purplecabbage> I'm really concerned
> with this switch for WP8 as it is breaking in nature. It changes how the
> Cordova WP8 JSON APIs behave and will cause a fair number of plugins to
> break. All plugins that break will have to make updates to support the new
> WP8 platform, and hence can't be used with the older version (3.x) of the
> WP8 platform.
>
> I would propose we don't make this disruptive change for WP8 which is a
> platform with limited runway as WP8.1+ should be using the Windows
> platform. WP8 should ideally be on life support and not making disruptive
> breaking changes that affect the plugin ecosystem.
>
> We should consider an incremental fix for file transfer and other plugins
> that need better support for JSON parsing instead of changing the platform
> itself: apache/cordova-plugin-file-transfer#72
> <https://github.com/apache/cordova-plugin-file-transfer/pull/72>
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/cordova-wp8/pull/79#issuecomment-101378234>.
>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-wp8 pull request: Added Newtonsoft json dll
Posted by brodybits <gi...@git.apache.org>.
Github user brodybits commented on the pull request:
https://github.com/apache/cordova-wp8/pull/79#issuecomment-99942506
Array.ConvertAll did not compile on Visual Studio Express 2013, and is apparently not supported for Metro or Silverlight. Figured out how to do this using Linq:
```diff
diff --git a/src/wp/File.cs b/src/wp/File.cs
index 941c9c8..085ef77 100644
--- a/src/wp/File.cs
+++ b/src/wp/File.cs
@@ -13,6 +13,7 @@
*/
using System;
+using System.Linq;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
@@ -695,7 +696,9 @@ public void readAsArrayBuffer(string options)
buffer = readFileBytes(filePath, startPos, endPos, isoFile);
}
- DispatchCommandResult(new PluginResult(PluginResult.Status.OK, buffer), callbackId);
+ int[] bufferAsIntArray = (from b in buffer select (int)b).ToArray();
+
+ DispatchCommandResult(new PluginResult(PluginResult.Status.OK, bufferAsIntArray), callbackId);
}
catch (Exception ex)
{
```
Please let me know if you want me to issue a PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-wp8 pull request: Added Newtonsoft json dll
Posted by purplecabbage <gi...@git.apache.org>.
Github user purplecabbage commented on the pull request:
https://github.com/apache/cordova-wp8/pull/79#issuecomment-99365163
Yeah @brodybits, will have another look at apache/cordova-plugin-file-transfer/pull/54
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-wp8 pull request: Added Newtonsoft json dll
Posted by purplecabbage <gi...@git.apache.org>.
Github user purplecabbage commented on the pull request:
https://github.com/apache/cordova-wp8/pull/79#issuecomment-99962513
nope, all auto build systems are hooped right now because of an issue with
Capitalized npm package names.
@purplecabbage
risingj.com
On Thu, May 7, 2015 at 10:54 AM, Chris Brody <no...@github.com>
wrote:
> Issued PR to fix CB-8968 (automatic build failed, don't think it is my
> fault)
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/cordova-wp8/pull/79#issuecomment-99960110>.
>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-wp8 pull request: Added Newtonsoft json dll
Posted by brodybits <gi...@git.apache.org>.
Github user brodybits commented on the pull request:
https://github.com/apache/cordova-wp8/pull/79#issuecomment-99354856
> So the apache/cordova-plugin-file-transfer#72 probably should be closed
then...
My point that apache/cordova-plugin-file-transfer#54 was an improvement
that was closed and rejected since we did not expect to add the Newtonsoft
JSON library. Now that we *do* include the Newtonsoft library, it is
**now** possible to include apache/cordova-plugin-file-transfer#54 (in
favor over apache/cordova-plugin-file-transfer#72 and
apache/cordova-plugin-file-transfer#82).
I hope [cordova-plugin-file-transfer](
https://github.com/apache/cordova-plugin-file-transfer) will consider
apache/cordova-plugin-file-transfer#54 again.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-wp8 pull request: Added Newtonsoft json dll
Posted by nikhilkh <gi...@git.apache.org>.
Github user nikhilkh commented on the pull request:
https://github.com/apache/cordova-wp8/pull/79#issuecomment-101388610
If we can avoid the breaking change it would be better and in this case we have a solution that works well to avoid it (apache/cordova-plugin-file-transfer#72). I understand Newtonsoft JSON is a better API and is more efficient.
Another alternative is for WP8 to provide new set of JSON APIs based on Newtonsoft JSON - The plugins that take dependency on these new APIs will have to use the engine tag and drop support for older versions of WP8. The older APIs will remain unchanged and a lot of plugins that are no longer maintained can continue to work.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-wp8 pull request: Added Newtonsoft json dll
Posted by brodybits <gi...@git.apache.org>.
Github user brodybits commented on the pull request:
https://github.com/apache/cordova-wp8/pull/79#issuecomment-99847116
> @brodybits follow these steps:
as documented in https://github.com/apache/cordova-plugin-test-framework. Could add a reference to README.md in the plugin for those who may be unfamiliar. Thanks @vladimir-kotikov.
> It looks like these changes brought new test failures on wp8:
> https://issues.apache.org/jira/browse/CB-8968
If we would restore the old code in `JsonHelper.Serialize` in `template/cordovalib/JSON/JsonHelper.cs` then the problem in CB-8968 goes away. Then:
Using `JsonConvert.SerializeObject` from `Newtonsoft.Json` in cordova-plugin-file *is working OK in the test*:
```diff
--- a/src/wp/File.cs
+++ b/src/wp/File.cs
@@ -22,7 +22,9 @@
using System.Text;
using System.Windows;
using System.Windows.Resources;
+
using WPCordovaClassLib.Cordova.JSON;
+using Newtonsoft.Json;
namespace WPCordovaClassLib.Cordova.Commands
{
@@ -771,7 +773,7 @@ public void readAsText(string options)
// JIRA: https://issues.apache.org/jira/browse/CB-8792
// Need to perform additional serialization here because NativeExecution is always trying
// to do JSON.parse() on command result. This leads to issue when trying to read JSON files
- var resultText = JsonHelper.Serialize(text);
+ var resultText = JsonConvert.SerializeObject(text);
DispatchCommandResult(new PluginResult(PluginResult.Status.OK, resultText), callbackId);
}
catch (Exception ex)
```
Using `JsonConvert.SerializeObject` from `Newtonsoft.Json` in `template/cordovalib/ScriptCallback.cs` is *working OK in the cordova-plugin-file test*:
```diff
--- a/template/cordovalib/ScriptCallback.cs
+++ b/template/cordovalib/ScriptCallback.cs
@@ -25,6 +25,8 @@
using WPCordovaClassLib.Cordova.JSON;
using System.Diagnostics;
+using Newtonsoft.Json;
+
namespace WPCordovaClassLib.Cordova
{
/// <summary>
@@ -65,7 +67,7 @@ public ScriptCallback(string function, string id, object msg, object value)
this.ScriptName = function;
String arg = String.Format("{{\"id\": {0}, \"msg\": {1}, \"value\": {2}}}",
- JsonHelper.Serialize(id), JsonHelper.Serialize(msg), JsonHelper.Serialize(value));
+ JsonConvert.SerializeObject(id), JsonConvert.SerializeObject(msg), JsonConvert.SerializeObject(value));
this.Args = new string[] { arg };
}
```
Using `JsonConvert.SerializeObject` from `Newtonsoft.Json` in `template/cordovalib/CordovaCommandCall.cs` gave me a problem with `file.spec.83` (blob test) the first time, then was passing the cordova-plugin-file tests when I tried it again (should be OK).
Using `JsonConvert.SerializeObject` from `Newtonsoft.Json` in `template/cordovalib/PluginResult.cs` gives me the errors as described in CB-8968.
Wondering if any other Apache Cordova or major third-party plugins would be affected as well.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-wp8 pull request: Added Newtonsoft json dll
Posted by brodybits <gi...@git.apache.org>.
Github user brodybits commented on the pull request:
https://github.com/apache/cordova-wp8/pull/79#issuecomment-99611924
> It looks like these changes brought new test failures on wp8:
> https://issues.apache.org/jira/browse/CB-8968
Can someone describe how I would run these cordova-plugin-file tests for wp8?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-wp8 pull request: Added Newtonsoft json dll
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cordova-wp8/pull/79
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
Re: [GitHub] cordova-wp8 pull request: Added Newtonsoft json dll
Posted by Jesse <pu...@gmail.com>.
Yes, absolutely. But, this is a major version jump, so breakage IS expected.
All plugins could have issues, the first step is to properly define the
'engines' in the plugin.xml to declare what versions you work with.
@purplecabbage
risingj.com
On Wed, May 6, 2015 at 10:28 AM, brodybits <gi...@git.apache.org> wrote:
> Github user brodybits commented on the pull request:
>
> https://github.com/apache/cordova-wp8/pull/79#issuecomment-99544620
>
> > It looks like these changes brought new test failures on wp8:
> > https://issues.apache.org/jira/browse/CB-8968
>
> @alsorokin I will take a look as well. Cannot promise I will find
> anything.
>
>
>
> ---
> If your project is set up for it, you can reply to this email and have your
> reply appear on GitHub as well. If your project does not have this feature
> enabled and wishes so, or if the feature is enabled but not working, please
> contact infrastructure at infrastructure@apache.org or file a JIRA ticket
> with INFRA.
> ---
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
> For additional commands, e-mail: dev-help@cordova.apache.org
>
>
[GitHub] cordova-wp8 pull request: Added Newtonsoft json dll
Posted by brodybits <gi...@git.apache.org>.
Github user brodybits commented on the pull request:
https://github.com/apache/cordova-wp8/pull/79#issuecomment-99544620
> It looks like these changes brought new test failures on wp8:
> https://issues.apache.org/jira/browse/CB-8968
@alsorokin I will take a look as well. Cannot promise I will find anything.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-wp8 pull request: Added Newtonsoft json dll
Posted by sgrebnov <gi...@git.apache.org>.
Github user sgrebnov commented on the pull request:
https://github.com/apache/cordova-wp8/pull/79#issuecomment-89258100
:+1: lgtm
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-wp8 pull request: Added Newtonsoft json dll
Posted by purplecabbage <gi...@git.apache.org>.
Github user purplecabbage commented on the pull request:
https://github.com/apache/cordova-wp8/pull/79#issuecomment-99137602
Today.
> On May 5, 2015, at 9:17 AM, Nikhil Khandelwal <no...@github.com> wrote:
>
> @purplecabbage - When do you plan to merge this. There is another related issue now causing WP8 mobilespec tests to crash because this has not been fixed within the platform: apache/cordova-plugin-file-transfer#82
>
> —
> Reply to this email directly or view it on GitHub.
>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-wp8 pull request: Added Newtonsoft json dll
Posted by alsorokin <gi...@git.apache.org>.
Github user alsorokin commented on the pull request:
https://github.com/apache/cordova-wp8/pull/79#issuecomment-99541999
It looks like these changes brought new test failures on wp8:
https://issues.apache.org/jira/browse/CB-8968
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-wp8 pull request: Added Newtonsoft json dll
Posted by purplecabbage <gi...@git.apache.org>.
Github user purplecabbage commented on the pull request:
https://github.com/apache/cordova-wp8/pull/79#issuecomment-99943449
Looks good, send away.
@purplecabbage
risingj.com
On Thu, May 7, 2015 at 10:17 AM, Chris Brody <no...@github.com>
wrote:
> Array.ConvertAll did not compile on Visual Studio Express 2013, and is
> apparently not supported for Metro or Silverlight. Figured out how to do
> this using Linq:
>
> diff --git a/src/wp/File.cs b/src/wp/File.cs
> index 941c9c8..085ef77 100644--- a/src/wp/File.cs+++ b/src/wp/File.cs@@ -13,6 +13,7 @@
> */
>
> using System;+using System.Linq;
> using System.Collections.Generic;
> using System.Diagnostics;
> using System.IO;@@ -695,7 +696,9 @@ public void readAsArrayBuffer(string options)
> buffer = readFileBytes(filePath, startPos, endPos, isoFile);
> }
> - DispatchCommandResult(new PluginResult(PluginResult.Status.OK, buffer), callbackId);+ int[] bufferAsIntArray = (from b in buffer select (int)b).ToArray();++ DispatchCommandResult(new PluginResult(PluginResult.Status.OK, bufferAsIntArray), callbackId);
> }
> catch (Exception ex)
> {
>
> Please let me know if you want me to issue a PR.
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/cordova-wp8/pull/79#issuecomment-99942506>.
>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-wp8 pull request: Added Newtonsoft json dll
Posted by brodybits <gi...@git.apache.org>.
Github user brodybits commented on the pull request:
https://github.com/apache/cordova-wp8/pull/79#issuecomment-99134840
> apache/cordova-plugin-file-transfer#82
apache/cordova-plugin-file-transfer#82 is a "quick fix" that could have been included already. Looking through the references, if this fix is accepted and released, I suspect it should be possible for [cordova-plugin-file-transfer](cordova-plugin-file-transfer) to include apache/cordova-plugin-file-transfer#54.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-wp8 pull request: Added Newtonsoft json dll
Posted by kim07 <gi...@git.apache.org>.
Github user kim07 commented on the pull request:
https://github.com/apache/cordova-wp8/pull/79#issuecomment-99206961
:+1:
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org