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 circular module dependency #1060

Merged
merged 2 commits into from
Sep 24, 2024

Conversation

rmunn
Copy link
Contributor

@rmunn rmunn commented Sep 23, 2024

Fix #814.

The build warnings ultimately came from the fact that $lib/forms/superforms.ts imports a single function from . (which in context is $lib/forms/index.ts), while at the same time $lib/forms/index.ts is importing and re-exporting everything, including the lexSuperForm function that's used all over our Svelte components.

There were three possible solutions:

  1. Configure Rollup (via vite.config.ts) to use the manualChunks option, requiring us to make decisions about what goes in every chunk. Not worth the cost and effort at this point.
  2. Change every place that imports lexSuperForm to import it from $lib/forms/superforms instead of from $lib/forms. This would have touched about a dozen files.
  3. Change one single import in $lib/forms/superforms.ts to import from ./utils instead of from ., thereby eliminating the circular import situation. This touches only a single line in a single file, but it eliminates the issue.

Obviously, I went with option 3.

@rmunn rmunn requested a review from hahn-kev September 23, 2024 06:39
@rmunn rmunn self-assigned this Sep 23, 2024
Copy link

github-actions bot commented Sep 23, 2024

UI unit Tests

12 tests   12 ✅  0s ⏱️
 4 suites   0 💤
 1 files     0 ❌

Results for commit f002ab6.

♻️ This comment has been updated with latest results.

hahn-kev
hahn-kev previously approved these changes Sep 23, 2024
myieye
myieye previously requested changes Sep 23, 2024
Copy link
Contributor

@myieye myieye left a comment

Choose a reason for hiding this comment

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

Nice fix. My personal opinion is that the comment should go.

frontend/src/lib/forms/superforms.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@myieye myieye left a comment

Choose a reason for hiding this comment

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

I did not intend my last review to be a change request....even though it sort of is 😉

@rmunn
Copy link
Contributor Author

rmunn commented Sep 24, 2024

@myieye - Sure. Now that we're all aware of the issue, the comment has (mostly) served its purpose. Go ahead and re-review when you get into the office.

@rmunn rmunn dismissed myieye’s stale review September 24, 2024 08:24

Approved by Kevin, no need to wait for Tim

@rmunn rmunn merged commit 5d01382 into develop Sep 24, 2024
9 checks passed
@rmunn rmunn deleted the chore/fix-javascript-bundle-warning-about-lexsuperform branch September 24, 2024 08:25
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.

frontend build warnings about chunking
3 participants