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

Resolve Circular Dependency #360

Merged
merged 2 commits into from
Mar 29, 2023
Merged

Resolve Circular Dependency #360

merged 2 commits into from
Mar 29, 2023

Conversation

afischer
Copy link
Member

@afischer afischer commented Mar 29, 2023

Description of Change

This PR removes some longstanding circular dependencies in the app without modifying app behavior. It also rearranges some tests to better reflect the new location of these dependencies.

I believe server/list.js > server/docs.js > server/formatter.js is causing the issues, but eventually we should work towards getting rid of all of them.

Cycles to resolve:

Detected via npx madge --circular .

  • server/cache.js > server/urlParser.js > server/list.js
  • server/cache.js > server/urlParser.js > server/list.js > server/docs.js
  • server/list.js > server/docs.js > server/formatter.js

Related Issue

Towards #361

Motivation and Context

With the release of v1.5.0, this bug could cause headaches for users after upgrading.

Checklist

  • Ran npm run lint and updated code style accordingly
  • npm run test passes
  • PR has a description and all contributors/stakeholder are noted/cc'ed
  • tests are updated and/or added to cover new code
  • relevant documentation is changed and/or added

@afischer afischer marked this pull request as ready for review March 29, 2023 14:14
@afischer afischer changed the title Resolve Circular Dependencies Resolve Circular Dependency Mar 29, 2023
server/list.js Outdated Show resolved Hide resolved
Copy link
Member

@isaacwhite isaacwhite left a comment

Choose a reason for hiding this comment

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

Makes sense to me! It probably is best for us to eliminate these circular dependencies to the extent that we can, but will mention that previously the way we avoided most of the runtime errors was by deferring the destructuring until just before the functions were needed.

That usually gives Node the time it needs to fully evaluate files on both sides of the import, so that all properties have been defined (so long as module.exports hasn't been reassigned)

@afischer afischer merged commit 73e244e into main Mar 29, 2023
@afischer afischer deleted the resolve-circ-deps branch March 29, 2023 21:39
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