Navigation Menu

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

Get rid of extraneous dependencies to resurrect the ---asseteditor page #8550

Closed
wants to merge 1 commit into from

Conversation

riknoll
Copy link
Member

@riknoll riknoll commented Oct 12, 2021

This looks way, way, scarier than it actually is! All this PR does is move things around a bit to make our dependency graph a little less complicated. Basically, the /---asseteditor page broke a while back when the cloudsync stuff went in. Obviously the cloudsync stuff shouldn't even be in the /---asseteditor page at all, so I had to untangle our imports a bit.

The big culprit was fireClickOnEnter which we use all over the place and is exported by sui.tsx (which itself requires app.tsx). I factored that out into a new file along with some of the asset editor utils that were also drawing in package.ts for no good reason.

To fix this, I wrote a script to trace the dependency graph and figure out the culprit. Here's how the dependency graph looked for the package.ts issues:

cloudsync-> /dialogs.js-> /container.js-> /headerbar.js-> /app.js-> /cloud.js-> /auth.js-> /core.js->
/compiler.js-> /workspace.js-> /package.js-> /components/assetEditor/store/assetEditorReducer.js->
/components/assetEditor/assetCard.js-> /components/ImageFieldEditor.js-> /assetEditor.js

Copy link
Contributor

@eanders-ms eanders-ms left a comment

Choose a reason for hiding this comment

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

Nice cleanup!

Copy link
Member

@jwunderl jwunderl left a comment

Choose a reason for hiding this comment

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

nice nice

would it possibly be worth checking in the script for future use?

@abchatra
Copy link
Contributor

@riknoll what is the status of this?

@riknoll
Copy link
Member Author

riknoll commented Dec 13, 2021

I need to resolve the conflicts, but i've been busy.

I'll close this and resurrect it later

@riknoll riknoll closed this Dec 13, 2021
@riknoll riknoll mentioned this pull request Dec 16, 2021
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.

None yet

5 participants