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: move next admin packages to core repo #5983

Merged
merged 24 commits into from
Jan 8, 2024

Conversation

kasperkristensen
Copy link
Contributor

@kasperkristensen kasperkristensen commented Jan 2, 2024

What

  • Move packages for next version of admin to core repo

Other

  • Since this PR introduces packages that depend on Vite 5, it also introduces @types/node@^20. We have never had a direct dependency on the types package for Node, and as far as I can see that has resulted in us using the types from Node.js@8, as those are a dependency of one of our dependencies. With the introduction of @types/node@^20, two of our packages had TS errors because they were using the NodeJS.Timer type, which was deprecated in Node.js@14. We should add specific @types/node packages to all our packages, but I haven't done so in this PR to keep it as clean as possible.
  • Q: @olivermrbl I've added the new packages to the ignore list for changeset, is this enough to prevent them from being published?

@kasperkristensen kasperkristensen requested a review from a team as a code owner January 2, 2024 13:29
Copy link

changeset-bot bot commented Jan 2, 2024

🦋 Changeset detected

Latest commit: bcb0554

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
create-medusa-app Patch
medusa-core-utils Patch
@medusajs/admin Patch

Not sure what this means? Click here to learn what changesets are.

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

Copy link

vercel bot commented Jan 2, 2024

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

3 Ignored Deployments
Name Status Preview Comments Updated (UTC)
api-reference ⬜️ Ignored (Inspect) Visit Preview Jan 8, 2024 9:05am
docs-ui ⬜️ Ignored (Inspect) Visit Preview Jan 8, 2024 9:05am
medusa-docs ⬜️ Ignored (Inspect) Visit Preview Jan 8, 2024 9:05am

@kasperkristensen kasperkristensen marked this pull request as draft January 2, 2024 13:31
@kasperkristensen kasperkristensen marked this pull request as ready for review January 3, 2024 12:58
@kasperkristensen kasperkristensen requested a review from a team as a code owner January 4, 2024 13:16
@olivermrbl
Copy link
Contributor

I've added the new packages to the ignore list for changeset, is this enough to prevent them from being published?

Yes, albeit with two caveats as per the changesets docs:

There are two caveats to this.

- If the package is mentioned in a changeset that also includes a package that is not ignored, publishing will fail.
- If the package requires one of its dependencies to be updated as part of a publish.

But I think we can manage.

Alternatively, we do as with the modules packages and just never include them in changesets.

Or thirdly, add private: true to the package.json 😄

packages/admin-next/admin-bundler/src/api/build.ts Outdated Show resolved Hide resolved
const mergedConfig = deepmerge(config1, config2)
mergedConfig.content = mergedContent

console.log(config1.presets, config2.presets)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
console.log(config1.presets, config2.presets)

packages/admin-next/admin-bundler/src/api/dev.ts Outdated Show resolved Hide resolved
packages/admin-next/dashboard/src/app.tsx Outdated Show resolved Hide resolved
}

if (!auth.user) {
console.log("redirecting");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
console.log("redirecting");

@olivermrbl
Copy link
Contributor

I think you might be missing prettier files to eliminate the semi-colons

@olivermrbl
Copy link
Contributor

Is this a long-running PR? I thought the idea was to merge this, so we can do the follow-up changes in "reviewable" chunks. Happy to do either approach.

@kasperkristensen
Copy link
Contributor Author

Is this a long-running PR? I thought the idea was to merge this, so we can do the follow-up changes in "reviewable" chunks. Happy to do either approach.

Merging now, I just didn't want to stop work until this was merged, but moving forward each piece of work will be one PR.

@kodiakhq kodiakhq bot merged commit f868775 into develop Jan 8, 2024
15 checks passed
@kodiakhq kodiakhq bot deleted the chore/move-admin-next-version branch January 8, 2024 09:26
This was referenced Jan 8, 2024
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.

2 participants