refactor(tasks, comments): core plugins #6333
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
ricokahler
force-pushed
the
refactor/core-plugins
branch
from
f3fe904
to
0401332
Compare
ricokahler
force-pushed
the
refactor/core-plugins
branch
from
0401332
to
e9c4bc8
Compare
ricokahler
force-pushed
the
refactor/core-plugins
branch
from
e9c4bc8
to
2071ccc
Compare
ricokahler
force-pushed
the
refactor/core-plugins
branch
from
2071ccc
to
06de830
Compare
ricokahler
force-pushed
the
refactor/core-plugins
branch
from
06de830
to
afbe437
Compare
ricokahler
force-pushed
the
refactor/core-plugins
branch
from
afbe437
to
535532f
Compare
|
No changes to documentation |
ricokahler
force-pushed
the
refactor/core-plugins
branch
from
535532f
to
75df605
Compare
|
Component Testing Report Updated Apr 17, 2024 6:23 PM (UTC)
|
ricokahler
force-pushed
the
refactor/core-plugins
branch
from
75df605
to
b71fc8e
Compare
ricokahler
force-pushed
the
refactor/core-plugins
branch
from
b71fc8e
to
9d20164
Compare
ricokahler
force-pushed
the
refactor/core-plugins
branch
from
1c3facc
to
a74f32d
Compare
pedrobonamin
approved these changes
Apr 17, 2024
ricokahler
force-pushed
the
refactor/core-plugins
branch
from
a74f32d
to
36212f4
Compare
ricokahler
force-pushed
the
refactor/core-plugins
branch
from
55b7748
to
58b3a97
Compare
jordanl17
pushed a commit
that referenced
this pull request
Apr 19, 2024
* refactor: load comments + tasks as default plugins * refactor(tasks): flatten folder structure * refactor(comments): flatten folder structure, move out of structure * refactor(comments): hoist inspector constant out of structure * refactor(tasks, comments): move into core * refactor: re-export comments from structure for backwards compatibility * refactor: fix build issues due to circular imports * refactor(core): consistent barrel exports * refactor(core): remove _internalBrowser * refactor(comments): hoist provider and decouple from structure * refactor(tasks): decouple from structure via IsLastPaneContext * test(e2e): fix e2e build by reverting relative imports --------- Co-authored-by: Espen Hovlandsdal <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The following PR:
coreto and makes them default/core plugins that are added to every sanity config.structure. This was done to preserve the seperation ofcoreandstructure. Since Tasks and Comments are now part ofcore, if this decoupling was not done, then much the structure tool would get imported due to the default plugins of tasks and comments.sanity/_internalBrowserentry point. The reasoning for this is that we already export a lot of internal code via the main export and it seems like we should either move all internals to that entry point or just remove it completely. The decision was to remove it since that's the easiest option that prevents the least amount of breakage.Some important notes:
Regarding default plugins
When testing this, I ran into a new bug that came as a result from making the
tasks()andcomments()plugins added by default. Certain context values were not present (<CommentsEnabledProvider/>and<TasksEnabledProvider />) which caused theuseCommentsEnabled()anduseTasksEnabled()hooks to throw. To prevent this issue, I gave these context values default values that have the contexts set toenabled: false. This is so that if there is no context provider (as in the case of the task's FormBuilder) for theseuse*Enabled()hooks, the feature will be assumed to be disabled instead of throwing.Decoupling Comments from
structureThough not ideal, the path I went down for decoupling Comments from
structurewas to invert the dependnecies and export theCommentProviderfrom core and import it intostructure. From there I was able to provide all the dependencies from structure that comments needed without having to import structure-only dependencies likeusePaneRouteranduseDocumentPane.In the future, we'd like to separate the concept of "the document" from structure and offer more document primitives available in core that are separate from structure. For now we may have to deal with structure depending on the CommentsProvider and comments populating it's document needs via
useDocumentPaneetc.Decoupling Tasks from
structureDecoupling Tasks from Structure has a similar story to decoupling Comments from Structure. The only dependency Tasks has toward structure is just
const {isLast} = usePane()so this was hoisted into a somewhat awkward<IsLastPaneProvider />. This is also a temporary thing and will be considered for those said primitive document APIs once we get some roadmap time. (cc @rexxars).Circular imports and barrel files
These are the bane of my existence. While moving these, I tried my best to make them consistent but I'd rather just remove them. They cause a lot of issues and I think we should invest some time in removing them. There were some interesting work arounds I had to do and I really don't have an explanation for why some of them work.
Some of the circular imports cause issues with the monorepo dev command, some of them cause issues with random jest tests, and others cause issues with vite in dev mode. The build is typically fine and rollup (as part of vite) usually knows how to deal with them but doesn't mean the rest of our tooling does 😅.
What to review
Testing
Tested manually and made sure
allalmost all CI passed and reviewed much of this large PR myself but definitely could use a lot of eyes on this one.Notes for release
N/A