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

chore: Update admin build/serve configuration #9584

Merged
merged 9 commits into from
Oct 15, 2024

Conversation

adrien2p
Copy link
Member

@adrien2p adrien2p commented Oct 15, 2024

Breaking changes

The outDir has been deprecated and wont be used anymore, instead all the path are computed internally following these rules

  • if admin is not disabled and the build command is run without the --admin-only flag, then the admin output dir will be .medusa/server/public/admin and it will be served from that same location from the medusa instance.
  • if admin is not disabled and the build command is run with the --admin-only flag, then the admin output dir will be .medusa/admin with the purpose of deploying the admin separately. ⚠️ (expect to receive a warning log)
  • if the admin is disabled and the build command is run with the --admin-only flag, then fallback to rule number 2
admin enabled medusa build --admin-only output dir
true true .medusa/admin ⚠️ (expect to receive a warning log)
true false .medusa/server/public/admin
false true .medusa/admin
false false none
// medusa-config.ts

{
  // ...
  admin: {
-    outDir: 'some/path'
  }
}

cc @kasperkristensen @sradevski @olivermrbl

Copy link

vercel bot commented Oct 15, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
medusa-dashboard ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 15, 2024 1:46pm
6 Skipped Deployments
Name Status Preview Comments Updated (UTC)
api-reference ⬜️ Ignored (Inspect) Oct 15, 2024 1:46pm
api-reference-v2 ⬜️ Ignored (Inspect) Visit Preview Oct 15, 2024 1:46pm
docs-ui ⬜️ Ignored (Inspect) Visit Preview Oct 15, 2024 1:46pm
docs-v2 ⬜️ Ignored (Inspect) Visit Preview Oct 15, 2024 1:46pm
medusa-docs ⬜️ Ignored (Inspect) Visit Preview Oct 15, 2024 1:46pm
resources-docs ⬜️ Ignored (Inspect) Visit Preview Oct 15, 2024 1:46pm

Copy link

changeset-bot bot commented Oct 15, 2024

⚠️ No Changeset found

Latest commit: c954a40

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@adrien2p adrien2p marked this pull request as ready for review October 15, 2024 12:40
@adrien2p adrien2p requested a review from a team as a code owner October 15, 2024 12:40
Copy link
Member

@sradevski sradevski left a comment

Choose a reason for hiding this comment

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

Nice!

packages/core/types/src/common/config-module.ts Outdated Show resolved Hide resolved
…thub.com:medusajs/medusa into chore/change-admin-serve-and-build-configuration
@kasperkristensen
Copy link
Contributor

  • if admin is not disabled and the build command is run with the --admin-only flag, then the admin output dir will be .medusa/admin with the purpose of deploying the admin separately

This case kind of doesn't make sense, you would never want to run the build command with --admin-only if you haven't disabled it as well, because in that case you end up with two admins.

I think its fine to keep the functionality, as it is the correct behaviour for case 3, but maybe it would make sense to log a warning like: "You are building the admin dashboard for standalone usage, but have not disabled the dashboard from being served along with your server. If you intend to host the dashboard separatly you should disable the admin in your medusa config".

@adrien2p
Copy link
Member Author

  • if admin is not disabled and the build command is run with the --admin-only flag, then the admin output dir will be .medusa/admin with the purpose of deploying the admin separately

This case kind of doesn't make sense, you would never want to run the build command with --admin-only if you haven't disabled it as well, because in that case you end up with two admins.

I think its fine to keep the functionality, as it is the correct behaviour for case 3, but maybe it would make sense to log a warning like: "You are building the admin dashboard for standalone usage, but have not disabled the dashboard from being served along with your server. If you intend to host the dashboard separatly you should disable the admin in your medusa config".

just added a warning as suggested 👍

@@ -0,0 +1,3 @@
export const ADMIN_SOURCE_DIR = "src/admin"
export const ADMIN_RELATIVE_OUTPUT_DIR = "./public/admin"
Copy link
Contributor

Choose a reason for hiding this comment

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

q: isn't the public folder typically associated with something else than build artefacts? maybe we can do ./dist/admin?

Copy link
Member Author

Choose a reason for hiding this comment

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

I dont have a strong opinion on this, the thinking was that it is served through express middleware and in general the files you serve through a middleware are often located in a public directory. But fine to move it to dist if you prefer

Copy link
Contributor

Choose a reason for hiding this comment

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

All good, let's keep it as is

@kodiakhq kodiakhq bot merged commit 84fa6cc into develop Oct 15, 2024
23 checks passed
@kodiakhq kodiakhq bot deleted the chore/change-admin-serve-and-build-configuration branch October 15, 2024 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants