nknorg / nkn-client-js

[Deprecated, use nkn-sdk-js instead] JavaScript implementation of NKN client
https://www.nkn.org
Apache License 2.0
55 stars 19 forks source link

Replace useless(?) Function constructor #43

Closed losnappas closed 5 years ago

losnappas commented 5 years ago

https://github.com/nknorg/nkn-client-js/blob/fd88803667b1f93532a7a6eb945a695375ab4683/lib/protocol/payloads_pb.js#L12

Please replace that line with.. say, var global = window;, as it is the same thing. It causes Firefox to go crazy over CSP. That would save me, and probably others, some headache with browserify and whatnot.

same thing https://github.com/nknorg/nkn-client-js/blob/master/lib/protocol/messages_pb.js#L12

yilunzhang commented 5 years ago

This is autogenerated code by protobuf: https://developers.google.com/protocol-buffers/docs/reference/javascript-generated

losnappas commented 5 years ago

should replace it anyways, if you ask me. Or maybe they have a method of generating it without function eval

yilunzhang commented 5 years ago

What problem are you getting into exactly? Typically modifying generated pb code is not a good way, we can see whether we can overcome it without having to manually modify the generated file every time we modify pb definition.

losnappas commented 5 years ago

Run this lib through browserify and you'll get a CSP error on Firefox, because of those 2 lines.

In fact, the files in /dist have this problem too.

yilunzhang commented 5 years ago

44 should solve this issue, @losnappas could you please confirm?

losnappas commented 5 years ago

Works, and I have an improvement.

instead doing the transformation in Gruntfile.js, move it into package.json, so that when I do require('nkn-client'), my browserify will know what to do, too. (So I won't need to explicitly use the dist files, which makes life a bit easier).

like this (package.json):

"browserify": {
    "transform": [
        ["browserify-replace", 
            {
                "replace": [{
                    "from": "var global = Function\\('return this'\\)\\(\\);",
                    "to": "var global = (function(){ return this  }).call(null);"
                }]
            }
        ]
    ]
  },

And you'll probably want to bump the version in package.json, too, if you forgot.

Made u a PR https://github.com/yilunzhang/nkn-client-js/pull/1 to save your time.

yilunzhang commented 5 years ago

Thanks! Merged your PR.