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

[refactor]: INVITE_BASEURL does not meet the semantics in flat-web #830

Closed
BlackHole1 opened this issue Jul 29, 2021 · 10 comments · Fixed by #1055
Closed

[refactor]: INVITE_BASEURL does not meet the semantics in flat-web #830

BlackHole1 opened this issue Jul 29, 2021 · 10 comments · Fixed by #1055
Assignees
Labels
enhancement New feature or request

Comments

@BlackHole1
Copy link
Collaborator

BlackHole1 commented Jul 29, 2021

INVITE_BASEURL Does not meet the semantics

Originally posted by @BlackHole1 in #829 (comment)

https://github.com/netless-io/flat/pull/834/files#diff-80cd6ff40d48acb0207a9e0780233787449517800f219c43a5d83f1569b344d1R95

@BlackHole1 BlackHole1 added the enhancement New feature or request label Jul 29, 2021
@BlackHole1 BlackHole1 changed the title refactor(flat-web): INVITE_BASEURL does not meet the semantics [refactor]: INVITE_BASEURL does not meet the semantics in flat-web Jul 29, 2021
@stale
Copy link

stale bot commented Sep 28, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Stale issues or PR label Sep 28, 2021
@stale stale bot closed this as completed Oct 5, 2021
@BlackHole1 BlackHole1 reopened this Oct 13, 2021
@BlackHole1 BlackHole1 removed the stale Stale issues or PR label Oct 13, 2021
@BlackHole1
Copy link
Collaborator Author

BlackHole1 commented Nov 3, 2021

INVITE_BASEURL => WEB_DOMAIN

@LitoMore
Copy link
Member

LitoMore commented Nov 3, 2021

Not sure we support https://example.com/SUB_DIRECTORY/replay/ROOM_TYPE/ROOM_UUID/OWNER_UUID/ kind of link or not.

If we want to support this kind of URL, we should not name it to the domain. base_url is better.

BTW, I noticed we already have the constant FLAT_WEB_DOMAIN in https://github.com/netless-io/flat/blob/main/web/flat-web/src/constants/process.ts.

Use INVITATION_BASE_URL or INVITATION_ENDPOINT instead will be better, IMO.

@BlackHole1
Copy link
Collaborator Author

BlackHole1 commented Nov 3, 2021

In #829, our requirement is to open the replay link, so using INVITATION/INVITE is not a good idea in this scenario.

This is because INVITE_BASEURL and FLAT_WEB_DOMAIN have a certain overlap.
Their domain names must be the same

see:

FLAT_SERVER_DOMAIN=flat-api-dev.whiteboard.agora.io
UPDATE_DOMAIN=https://flat-storage.oss-cn-hangzhou.aliyuncs.com/versions
FLAT_WEB_DOMAIN=flat-web-dev.whiteboard.agora.io

cc @LitoMore

@BlackHole1
Copy link
Collaborator Author

Should we only keep FLAT_WEB_DOMAIN? Or, keep FLAT_WEB_DOMAIN and give it a better name, e.g: WEB_DOMAIN?

@LitoMore
Copy link
Member

LitoMore commented Nov 3, 2021

The link https://{DOMAIN} with a protocol (https:) called URL or endpoint.

It's OK to use domain if it does not have the protocol part.

You could try this in Node.js REPL:

> new URL('https://exmaple.com:8080/test/test/file.ext')

The result will be:

URL {
  href: 'https://exmaple.com:8080/test/test/file.ext',
  origin: 'https://exmaple.com:8080',
  protocol: 'https:',
  username: '',
  password: '',
  host: 'exmaple.com:8080',
  hostname: 'exmaple.com',
  port: '8080',
  pathname: '/test/test/file.ext',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}

The domain and the hostname are the same thing. Hope this helps you understand the URL.

@BlackHole1
Copy link
Collaborator Author

Thanks for the correction, that might be better called FLAT_WEB_ORIGIN / FLAT_WEB_URL / FLAT_WEB_BASE_URL?

@LitoMore
Copy link
Member

LitoMore commented Nov 3, 2021

The WEB_URL sounds like a full URL. Origin is only used for the HTTP regions, it's not a good choice for URL naming.

FALT_WEB_BASE_URL looks good.

@BlackHole1
Copy link
Collaborator Author

BlackHole1 commented Nov 3, 2021

Good. Would anyone like to try and submit a PR for this issue? @netless-io/developers

@LitoMore
Copy link
Member

LitoMore commented Nov 3, 2021

OK, let me solve this.

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

Successfully merging a pull request may close this issue.

3 participants