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

Avoid circular imports #6814

Merged
merged 12 commits into from
Aug 7, 2024
Merged

Avoid circular imports #6814

merged 12 commits into from
Aug 7, 2024

Conversation

kraenhansen
Copy link
Member

@kraenhansen kraenhansen commented Jul 25, 2024

What, How & Why?

This closes #6565 by refactoring imports away from the "internal" pattern (which we introduced when we used Rollup to bundle into a single file) and introducing an indirect object which gets classes injected once they're initialized. This also untangles the recursively referencing collections accessors introduced with collections in mixed, into separate files.

This also adds a dependency in the "madge" tool enabling a new script to check for circular imports:

npm run check-circular-imports --workspace realm

Although this still fails with three cases of circular references 🤔 I need to decide how to deal with those before adding it as a command which is ran as part of our lint workflow on CI.

☑️ ToDos

  • 📝 Changelog entry
  • 📝 Compatibility label is updated or copied from previous entry
  • 📝 Update COMPATIBILITY.md
  • 🚦 Tests
  • 📦 Updated internal package version in consuming package.jsons (if updating internal packages)
  • 📱 Check the React Native/other sample apps work if necessary
  • 💥 Breaking label has been applied or is not necessary
  • 🔔 Mention @realm/devdocs if documentation changes are needed

@kraenhansen kraenhansen requested a review from gagik July 26, 2024 05:41
@kraenhansen kraenhansen marked this pull request as ready for review July 26, 2024 05:42
@kraenhansen kraenhansen added no-changelog no-jira-ticket Skip checking the PR title for Jira reference T-Internal labels Jul 26, 2024
@kraenhansen
Copy link
Member Author

@gagik I recognize these are a lot of changes, but most were pretty mechanical. I'd love to get this in soon-ish to avoid the codebase from diverging while I'm PTO 🙏

@kraenhansen kraenhansen mentioned this pull request Jul 26, 2024
8 tasks
Copy link
Contributor

@gagik gagik left a comment

Choose a reason for hiding this comment

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

Nice changes!

packages/realm/src/Counter.ts Show resolved Hide resolved
packages/realm/src/assert.ts Show resolved Hide resolved
Copy link
Contributor

@kneth kneth left a comment

Choose a reason for hiding this comment

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

The imports are much easier to read. And I like that you have isolated the collection accessors in separate files.

@gagik gagik merged commit 9364039 into main Aug 7, 2024
40 of 41 checks passed
@gagik gagik deleted the kh/avoid-circular-imports branch August 7, 2024 10:46
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes no-changelog no-jira-ticket Skip checking the PR title for Jira reference T-Internal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bundling with Metro warns with "Require cycles are allowed, but can result in uninitialized values."
3 participants