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

Remove an import from cursorless-engine/src/util to src/processTargets. #2016

Closed
wants to merge 5 commits into from

Conversation

bjaspan
Copy link
Contributor

@bjaspan bjaspan commented Nov 10, 2023

Generally, code in src/utils should not be importing from src/processTargets. This cleans one instance of this up.

It also changes some imports from using the index.ts file to the direct file exporting the thing.

I do not think this will actually lead to a solution to the circular-import problem I'm having because ultimately there are still clearly circular file dependencies in this code, but removing the utils -> processTargets import still seems like a small improvement.

@@ -1,4 +1,4 @@
import { PlainTarget } from "../processTargets/targets";
import { PlainTarget } from "../processTargets/targets/PlainTarget";
Copy link
Member

@pokey pokey Nov 10, 2023

Choose a reason for hiding this comment

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

As discussed, you should only switch from index file to direct import when both files are descendants of the folder containing the index file. This import could be what is breaking the build, because it subverts the barrel hack that we have to serialize the imports in target dir

@bjaspan
Copy link
Contributor Author

bjaspan commented Nov 14, 2023

I am revoking this PR, perhaps temporarily.

@bjaspan bjaspan closed this Nov 14, 2023
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.

2 participants