search

mymonero / mymonero-core-js

The JS library containing the Monero crypto plus lightwallet functions behind the official MyMonero apps
BSD 3-Clause "New" or "Revised" License
101 stars 103 forks source link

fixed bug with second argv in electron built application #38

Closed alidtoran closed 5 years ago

alidtoran commented 6 years ago

Built in electron application crashes on production env because the second parameter of process["argv"] is undefined

paulshapiro commented 6 years ago

Not seeing this in our electron app

paulshapiro commented 6 years ago

This fix looks like it would be fine, functionally.

But instead of "" I think it will be more rigorous to go with a null instead. Note how this parameter's value will be used in the actual program. It will be initialized to a default value if it's falsy. "" is falsy but not explicitly and therefore ambiguously so.

Secondly I would like to mention that this is not the appropriate way to fix this problem as this file is automatically generated and will be replaced shortly. The fix is to confirm when emscripten fixed the problem, if they did, and whether that fix was placed in before the version of emscripten that must be used to build a Node.JS compatible build.

paulshapiro commented 6 years ago

Before merging though I should confirm why you're experiencing the issue when we're not. Version or invocation environment difference?

waterelder commented 6 years ago

"electron"="1.7.5" "electron-builder"="^20.0.7" The problem is on all platforms. screenshot

paulshapiro commented 6 years ago

That's not why it's happening though. Our desktop app https://github.com/mymonero/mymonero-app-js has never seen this issue through a superset of versions. For that reason, it doesn't matter but it might be useful to know that we use a more modern version of electron-builder. You might try upgrading? Maybe there was a bug.

alidtoran commented 6 years ago

We upgraded electron-builder to 20.14.7. But it didn't solve the problem

paulshapiro commented 6 years ago

So that begs the question

paulshapiro commented 6 years ago

Please verify this is still an issue for you in the cpp branch

paulshapiro commented 5 years ago

Is this still an issue?

alidtoran commented 5 years ago

we have finished work on this project so issue is not relevant any more =)

paulshapiro commented 5 years ago

Moved onto something else? Or, if you completed it, did you find a resolution then?

alidtoran commented 5 years ago

We used the forked version. Maybe it has been replaced by something else.