Skip to content
This repository has been archived by the owner on Feb 23, 2021. It is now read-only.

Update deprecated flag and homepage field #358

Merged
merged 1 commit into from
Jun 3, 2018
Merged

Conversation

valentinewallace
Copy link
Contributor

I copied the changes over from my Ubuntu VM so hopefully they work.

Additionally, I had to cp my lnd and btcd binaries into the assets/build/linux folder for the package to build correctly (symlinks didn't work). Not sure if this should be put into the README or what?

Copy link
Contributor

@tanx tanx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@valentinewallace just a few questions.

@@ -2,7 +2,7 @@
"name": "lightning-app",
"version": "0.1.0",
"description": "Lightning Wallet Application",
"homepage": "https://lightning.engineering",
"homepage": "./",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The npm docs say this attribute is for the project homepage? https://docs.npmjs.com/files/package.json#homepage

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/facebook/create-react-app/blob/ca7d227ae0354e04941ba0e4f76c0bcbf3050dfa/config/webpack.config.prod.js#L20-L23 create-react-app uses package.json's homepage field when it creates the webpack config as webpack's publicPath, which is what the static files are served from. Got this fix from here: facebook/create-react-app#165 lmk if it isn't acceptable

@@ -16,7 +16,7 @@
"test:react": "react-scripts test --env=jsdom",
"eject": "react-scripts eject",
"electron-dev": "concurrently \"BROWSER=none npm start\" \"wait-on http://localhost:3000 && electron --enable-sandbox .\"",
"electron-pack": "build --em.main=build/electron.js",
"electron-pack": "build --c.extraMetadata.main=build/electron.js",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this fixes the build?

Copy link
Contributor Author

@valentinewallace valentinewallace Jun 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think both changes are necessary

@tanx
Copy link
Contributor

tanx commented Jun 1, 2018

Additionally, I had to cp my lnd and btcd binaries into the assets/build/linux folder for the package to build correctly (symlinks didn't work). Not sure if this should be put into the README or what?

Do you mean during development or for the build? During development you just have to make sure you have lnd/btcd in your PATH. This is explained in the installation guide which is already linked in the README: https://github.com/lightninglabs/lightning-app/blob/master/README.md#developing-locally

@valentinewallace
Copy link
Contributor Author

Do you mean during development or for the build? During development you just have to make sure you have lnd/btcd in your PATH. This is explained in the installation guide which is already linked in the README: https://github.com/lightninglabs/lightning-app/blob/master/README.md#developing-locally

I mean for the build of the packaged app, but in both cases i needed to do this. i'm confused why it would work otherwise due to the structure of getProcessName in lnd-child-process -- messaging you

@tanx
Copy link
Contributor

tanx commented Jun 3, 2018

@valentinewallace Ok. Tested this. Looks good 👍

@tanx tanx merged commit 582042b into master Jun 3, 2018
@tanx tanx deleted the fix-electron-build branch June 3, 2018 07:52
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants