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

Add @bull-board/ui as peerDep of @bull-board/api #565

Merged
merged 2 commits into from
Apr 30, 2023
Merged

Add @bull-board/ui as peerDep of @bull-board/api #565

merged 2 commits into from
Apr 30, 2023

Conversation

JasonMan34
Copy link
Contributor

This closes #564

@@ -36,6 +36,9 @@
"bullmq": "^1.80.6",
"ioredis-mock": "^7.2.0"
},
"peerDependencies": {
"@bull-board/ui": "5.1.1"
Copy link
Owner

Choose a reason for hiding this comment

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

lets not restrict the version here, make it *

Copy link
Contributor Author

@JasonMan34 JasonMan34 Apr 30, 2023

Choose a reason for hiding this comment

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

Wasn't sure about compatibility issues so I copied the version from the inverse dependency (ui's dependency on api@5.1.1)

Changed to *

@felixmosh felixmosh merged commit cb894d4 into felixmosh:master Apr 30, 2023
@felixmosh
Copy link
Owner

Released in v5.1.2, can you try this version?

@JasonMan34
Copy link
Contributor Author

JasonMan34 commented Apr 30, 2023

Works now :)

Although the solution feels off to me. ui's dependency of api is obvious, but the reverse dependency only exists because of this code:

export function createBullBoard({
  queues,
  serverAdapter,
  options = { uiConfig: {} },
}: {
  queues: ReadonlyArray<BaseAdapter>;
  serverAdapter: IServerAdapter;
  options?: BoardOptions;
}) {
  const { bullBoardQueues, setQueues, replaceQueues, addQueue, removeQueue } = getQueuesApi(queues);
  const uiBasePath = path.dirname(eval(`require.resolve('@bull-board/ui/package.json')`));

  serverAdapter
    .setQueues(bullBoardQueues)
    .setViewsPath(path.join(uiBasePath, 'dist'))
    .setStaticPath('/static', path.join(uiBasePath, 'dist/static'))
    .setUIConfig(options.uiConfig)
    .setEntryRoute(appRoutes.entryPoint)
    .setErrorHandler(errorHandler)
    .setApiRoutes(appRoutes.api);

  return { setQueues, replaceQueues, addQueue, removeQueue };
}

I think there are a few approaches that would be better than the current peerDep solution:

  1. Perhaps if createBullBoard is inherently coupled with @bull-board/ui, it should be exported from there? That would remove the cyclic dependency and resolve the issue in a more elegant way
  2. Decouple createBullBoardcompletely from @bull-board/ui entirely, and accept another required parameter uiBasePath. Then @bull-board/ui could export a uiBasePath to be used when calling createBullBoard
  3. Export shared dependencies (KeyOf, STATUSES, AppQueue, etc.) to a @bull-board/core package and have both ui and api depend on core (Remove ui's dependency on api). Then have api rely on ui directly (Regular dependency over peerDependency)

@felixmosh
Copy link
Owner

Thanx for the feedback.

  1. createBullBoard is a builder of api, it should be on the server side part.
  2. I've thought about it, but then each user of the lib will need to pass the same piece of code
  3. Good possible solution, I'll consider this refactor, thanx

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.

Undeclared peer dependency between @bull-board/api and @bull-board/ui breaks with yarn's PnP system
2 participants