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

Granular app locator #1124

Merged
merged 13 commits into from
Mar 9, 2020
Merged

Granular app locator #1124

merged 13 commits into from
Mar 9, 2020

Conversation

bpierre
Copy link
Contributor

@bpierre bpierre commented Oct 14, 2019

Allows to set the app locator in a more granular way:

  • Bridges can be defined individually, using a comma-separated list.
  • An app can be defined using its app ID directly, its ENS name ending in (.aragonpm.eth), or its known name if it exists (Finance, TokenManager, etc.).
  • Known names also have a known location (http://localhost:<port>/), that gets used by default if not provided.
  • If only a port is provided, localhost is used as a domain.

Load all known apps from the local host, as before:

ARAGON_APP_LOCATOR=local npm start

Only load the finance app from the local host (known port):

ARAGON_APP_LOCATOR=Finance npm start

Load the finance app from localhost:1234:

ARAGON_APP_LOCATOR=Finance:1234 npm start

Load the finance app and the Tokens app from the local host (known port):

ARAGON_APP_LOCATOR=Finance,TokenManager npm start 

Load the 0x6b20… app (app ID) from localhost:3333:

ARAGON_APP_LOCATOR=0x6b20…:3333 npm start

Load the my-app.aragonpm.eth app (ENS ID) from localhost:

ARAGON_APP_LOCATOR=my-app.aragonpm.eth npm start

Load the Voting app from https://example.com:4444:

ARAGON_APP_LOCATOR=Voting:https://example.com:4444/

Also updated @babel/core, @babel/preset-env and @babel/preset-react.
Allows to get the app locator based on the asset bridge provided.
src/known-app-ids.js Show resolved Hide resolved
src/environment.js Outdated Show resolved Hide resolved
src/app-locator.js Show resolved Hide resolved
src/app-locator.js Show resolved Hide resolved
// "0x6b20…:3333": load the app with 0x6b20… ID from localhost & the 3333 port.
// "Voting:http://example.com:4444/": load the Voting app from example.com & the 4444 port.
//
export default function getAppLocator(assetBridge) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about calling this an "override"?

We still fallback to loading apps that aren't included in this by using their IPFS content.

Copy link
Contributor Author

@bpierre bpierre Oct 18, 2019

Choose a reason for hiding this comment

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

Yes 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed it parseAppLocator(), I think it makes more sense and doesn’t conflict with getAppLocator() which returns the string from the env. What do you think?

src/app-locator.js Show resolved Hide resolved
@stale stale bot added the abandoned label Nov 17, 2019
@aragon aragon deleted a comment from stale bot Nov 17, 2019
@stale stale bot removed the abandoned label Nov 17, 2019
@stale stale bot added the abandoned label Dec 17, 2019
@aragon aragon deleted a comment from stale bot Dec 17, 2019
@stale stale bot removed the abandoned label Dec 17, 2019
@stale stale bot added the abandoned label Jan 16, 2020
@aragon aragon deleted a comment from stale bot Jan 16, 2020
@stale stale bot removed the abandoned label Jan 16, 2020
@stale stale bot added the abandoned label Feb 15, 2020
@aragon aragon deleted a comment from stale bot Feb 15, 2020
@stale stale bot removed the abandoned label Feb 15, 2020
@bpierre bpierre changed the title Custom asset bridge Granular app locator Mar 9, 2020
}

// Get the default URL for this appId (when location=local).
if (!location || location === 'local') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm wondering about this; right now, this would force all apps to use the default local override even if they're not part of the list.

I think we should still be checking that DEFAULT_LOCAL_URLS[appId] exists?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mmm we don’t have an appId at this stage? We are only getting the list of defined app locators. Then in appBaseUrl(), we only override if the app ID exists in that object.

Copy link
Contributor

@sohkai sohkai left a comment

Choose a reason for hiding this comment

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

Let's also add some documentation to CONFIGURATION.md 😄

@bpierre
Copy link
Contributor Author

bpierre commented Mar 9, 2020

Let's also add some documentation to CONFIGURATION.md

Done!

docs/CONFIGURATION.md Outdated Show resolved Hide resolved
Co-Authored-By: Brett Sun <qisheng.brett.sun@gmail.com>
src/known-app-ids.js Outdated Show resolved Hide resolved
Copy link
Contributor

@sohkai sohkai left a comment

Choose a reason for hiding this comment

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

Awesome 🙌🙌

So excited this is finally done :).

@bpierre bpierre merged commit e97d046 into master Mar 9, 2020
@bpierre bpierre deleted the custom-asset-bridge branch March 9, 2020 21:16
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