-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Cleanup extensions dependencies #19922
Conversation
Different PR!
🦋 Changeset detectedLatest commit: 65e1c5f The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 packages
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 |
@paescuj @azrikahar, gonna take an educated risk on this and merge it. I'd still like a brief retrospective review if possible, but less of a priority 👍🏻 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in general! Just some tiny questions/todos from what I can observe.
I believe we'll need to add a license file as well 👍
}; | ||
|
||
export type BundleExtension = ExtensionBase & { | ||
type: 'bundle'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understandably this isn't changed by this PR at all, but retroactively I wonder whether 'bundle'
should be BundleExtensionType
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it should, I have some more cleaning up to do so I'll take it into account for that 👍🏻
"./node": "./dist/node/index.js", | ||
"./package.json": "./package.json" | ||
}, | ||
"main": "dist/index.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be misinterpreting this, but should it be dist/shared/index.js
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix ready at #19926 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we'll need to update this readme to describe @directus/extensions
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops! Copy paste issue
* Adjust release types * Update package.json file * Adjust readme * Align type def for BundleExtension Co-authored-by: Azri Kahar <42867097+azrikahar@users.noreply.github.com> * Add license --------- Co-authored-by: Azri Kahar <42867097+azrikahar@users.noreply.github.com>
* Setup boilerplate for extensions page * Allow reading all extensions from the root endpoint * Add basic sidebar info detail * Render list of extensions * Move extensions manager to folder * Move stuff related to extensions to @directus/extensions * Import extensions utils/types/etc from ext. package * Resolve last imports * Temp remove settings pane Different PR! * Add changeset * Run formatter * Fix import in test
* Adjust release types * Update package.json file * Adjust readme * Align type def for BundleExtension Co-authored-by: Azri Kahar <42867097+azrikahar@users.noreply.github.com> * Add license --------- Co-authored-by: Azri Kahar <42867097+azrikahar@users.noreply.github.com>
Scope
What's changed:
@directus/extensions
package/extensions
Potential Risks / Drawbacks
@directus/utils
/@directus/types
/@directus/constants
might have to import from@directus/extensions
now. All types exported through@directus/extensions-sdk
remain the same.Review Questions