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

feat: Expose web ui router for external server, and switch to *http.ServeMux from gocraft/web #44

Merged
merged 13 commits into from
Oct 3, 2024

Conversation

vin-rmdn
Copy link

@vin-rmdn vin-rmdn commented Sep 24, 2024

Resolves #43

@vin-rmdn
Copy link
Author

vin-rmdn commented Sep 26, 2024

Action items from discussion with @chopraanmol1 via DM:

  • Do not expose server’s router, this looks like a hack. Instead, let’s create a new method NewRouter which returns *web,Router and NewServer can call the same method internally
    • For new router constructor, let’s accept dependency and build client
    • Please check, on the web UIs what field from Server is used, and add that accordingly to the router constructor
      • I'm opting to have NewServer depend on NewRouter instead
  • On web side let’s try to use relative path to avoid the need of knowing or replacing path prefix.
  • For context struct, since we’re having router constructor, let’s change the field inside context from Server to Client

Proposed structure:

func NewServer(namespace string, pool *redis.Pool, hostPort string) *Server {
	...
	client = work.NewClient(namespace, pool)
	router = buildRouter(client)
	...
}

func NewRouter(namespace string, pool *redis.Pool) *web.Router {
	...
	client = work.NewClient(namespace, pool)
	return buildRouter(client)	
}

func buildRouter(*work.Client) *web.Router {
	....
}

EDIT 2024/09/30: relative path is doable

@vin-rmdn vin-rmdn changed the title feat: web ui to expose router for external server feat: web ui public router constructor for external server attachment Sep 26, 2024
@vin-rmdn
Copy link
Author

vin-rmdn commented Sep 30, 2024

More action items with @chopraanmol1

  • Let's do relative path for our JS-based worker UI (it will work if we do /prefix/ instead of /prefix) and replace it for all HTML and JS urls
  • Remove mention of path prefix
    • on HTML and JS
    • on Go code
      • *Currently not possible due to web.Router needing the specific prefix for subrouting, else the router will return 404
  • Revert the templating change
  • Refactoring changes suggestions:
    • Rename NewRouter to BuildRouter
    • Instead of doing RouterOptions, let's pass *web.Router, which removes any path-specific code from the library
      • Adding *web.Router as parameter will explicitly state dependency of gocraft/web for the router factory, and this will break compatibility with other routers (e.g., Mux, Echo) and require dependent service to use another routing library as well
    • For this new method, add comments that for a trailing / is needed for the webui to work as expected
  • To simplify review, move back few of the file back to webui.go: moved context back to webui file.

@chopraanmol1 chopraanmol1 self-requested a review October 3, 2024 13:01
@chopraanmol1 chopraanmol1 changed the title feat: web ui public router constructor for external server attachment feat: Expose web ui router for external server, and switch to *http.ServeMux from gocraft/web Oct 3, 2024
@chopraanmol1 chopraanmol1 merged commit ad0f5c1 into gojek:master Oct 3, 2024
3 checks passed
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.

Expose work web UI inside a path prefix
2 participants