Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move the default network setting outside of built bundles - Closes #491 #538

Merged
merged 17 commits into from
Mar 13, 2018

Conversation

faival
Copy link
Contributor

@faival faival commented Mar 8, 2018

What was the problem?

A flag is used at build time to set testNet or mainNet as the preferred network to show, this should be changed to be set clientside

How did I fix it?

  • replace npm scripts goals used by webpack build approach
  • replace usage of env variables in app codebase
  • provide a way to copy built assets and only replace a flag in main html

How to test it?

  • run the following steps:
npm install
npm run build
npm run build:testnet
  • then open the generated app built in ./app/build/index.html
    (assert when passing ?showNetwork=true) points to mainnet
  • then open the generated app build in ./app/build-testnet/index.html
    (assert when passing ?showNetwork=true) points to testnet

assert for both builds that wen actively setting a value on local storage of browser with:

localStorage.setItem('defaultNetwork', 'mainnet' || 'testnet' || 'customNode'), 

then that value is used next time the application is reloaded.

Review checklist

@faival faival self-assigned this Mar 8, 2018
src/index.html Outdated
@@ -257,6 +257,14 @@

</style>
<link rel="icon" type="image/png" href="./assets/images/LISK.png" />
<script type="text/javascript">
var defaultNet = "mainnet";//defaultNetwork
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add another comment to explain that //defaultNetwork is used npm run build:testnet. Otherwise, seeing just this file makes it look like a useless comment that I would delete if I didn't know the context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

src/index.html Outdated
var defaultNet = "mainnet";//defaultNetwork
(function(targetNet) {
if (window.localStorage){
window.localStorage.setItem('defaultNetwork', defaultNet);
Copy link
Contributor

Choose a reason for hiding this comment

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

The if could also check if default network is already defined in local storage. And don't set it in that case. That way, we can set defaultNetwork on localhost when developing (when needed) and it won't be overridden on each page reload.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@faival faival removed the ✅ ready label Mar 12, 2018
Copy link
Contributor

@slaweet slaweet left a comment

Choose a reason for hiding this comment

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

Thank you, Pablo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants