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

Added support for non root (home) deployment of bit-server #4923 #4958

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

pgangwani
Copy link

@pgangwani pgangwani commented Oct 13, 2021

Fixes Issue

#4923

How to test

  1. Clone, checkout https://github.com/pgangwani/bit-server-static-public-path-issue
  2. Above repo represent setup where bit-server is not deploeyed on root / of the domain
  3. Make sure we setup bit locally with link name bit-local
# clone
git clone https://github.com/teambit/bit.git
cd bit

# install
rm -rf node_modules public/bit
bit install # this might take a long time
bit compile

# probably not needed:
# npm run build
# npm run build:types 
## OR
# npm run build:windows 

# one time registration
npm run dev-link bit-local
# OR
npm run dev-link:windows bd

# you will then be able to run:
bd -v
# output will be "last-tag 0.0.516"
# (unlike bit -v, which outputs "0.0.516"

# etc 
> ~/project/example-project$
bd start --rebuild
  1. Run npm run reproduce:issue
  2. Goto /bit-dev/ route and test the server with & without pushing components.

Proposed Changes

  • Need help on how to remove hardcoded values to be coming from configs
  • Test all use case
  • Review comments for sure

@pgangwani pgangwani changed the title # Added support for non root (home) deployment of bit-server #4923 Oct 13, 2021
@KutnerUri
Copy link
Contributor

very nice! need to

  • replace hard coded values with configuration (worst case I'll do it)
  • remove unrelated files (yarn.lock, prettiers)

@KutnerUri KutnerUri marked this pull request as draft October 13, 2021 14:54
@pgangwani
Copy link
Author

pgangwani commented Oct 13, 2021

very nice! need to

  • replace hard coded values with configuration (worst case I'll do it)
  • remove unrelated files (yarn.lock, prettiers)

Removed unrelated file.
As discussed over slack, I really need to connect stuff. I could connect only static/ folder till now and was able to make changes in respective to files to make it work in /bit-dev/ route

@KutnerUri KutnerUri self-requested a review October 14, 2021 08:19
Comment on lines 69 to 71
const wsUrl = `${isInsecure ? 'ws:' : 'wss:'}//${location.host}/subscriptions`;
const wsUrl = `${isInsecure ? 'ws:' : 'wss:'}//${location.host}/bit-dev/subscriptions`;

const client = this.graphqlUI.createClient('/graphql', { state, subscriptionUri: wsUrl });
const client = this.graphqlUI.createClient('/bit-dev/graphql', { state, subscriptionUri: wsUrl });
Copy link
Contributor

Choose a reason for hiding this comment

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

need to pass basename from graphql/ui aspect

const defaultServerUrl = `/api/${component.id.toString()}/~aspect/preview/`;
const defaultServerUrl = `/bit-dev/api/${component.id.toString()}/~aspect/preview/`;
Copy link
Contributor

@KutnerUri KutnerUri Oct 14, 2021

Choose a reason for hiding this comment

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

need to pass basename (from where..?) as prop.

Where to get the basename prop though?
maybe we can it from react-router context?

worse case we can set a global react-context from the UI aspect, something like:

const { basename, apiPath } = useContext(???);

const defaultServerUrl = `${apiPath}/${component.id.toString()}/~aspect/preview/`;

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking of adding basename to our internal routing system, as part of the changes I'm going to do to @teambit/base-ui.routing.routing-provider anyways.

It would be more elegant.

Then again, I'm not sure it will work here, because explicitUrl could have it's own url.

routing = Routing.url,
children,
location,
basename = '/bit-dev/',
Copy link
Contributor

Choose a reason for hiding this comment

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

default should be undefined (if possible), or / otherwise.

the "bit-dev" should come from the UI aspect

Comment on lines 525 to 526
// Changing to relative path which should work in all cases
publicPath: process.env.ASSET_PATH || './',
Copy link
Contributor

Choose a reason for hiding this comment

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

ooh this looks interesting, how does it work

workspace.jsonc Outdated Show resolved Hide resolved
@KutnerUri
Copy link
Contributor

forwarded publicPath to graphql aspect and react-router aspect.
Only left to deduce the basename for preview iframe, I need to think how to do that.

@pgangwani
Copy link
Author

pgangwani commented Oct 18, 2021

forwarded publicPath to graphql aspect and react-router aspect. Only left to deduce the basename for preview iframe, I need to think how to do that.

Thanks @KutnerUri for interim update. Will checkout latest and test. Can you share the steps how can I test when I have just scope.json ?

@KutnerUri KutnerUri marked this pull request as ready for review October 28, 2021 15:47
KutnerUri
KutnerUri previously approved these changes Oct 28, 2021
GraphqlPreview.provider = ([previewPreview]: [PreviewPreview]) => {
const graphqlPreview = new GraphqlPreview();

previewPreview.registerRenderContext(() => graphqlPreview.getConfig());
Copy link
Contributor

Choose a reason for hiding this comment

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

forward gql config to preview

@@ -57,6 +60,19 @@ export class GraphqlUI {
return client;
}

getConfig = (): GqlConfig => {
Copy link
Contributor

Choose a reason for hiding this comment

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

dynamic gql endpoints, based on configuration

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.

3 participants