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

fix: Remove circular dependency #741

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

edeustace
Copy link

When bundling mathjax - you are warned that there is a circular
dependency:

code: CIRCULAR_DEPENDENCY, message: Circular dependency: node_modules/mathjax-full/js/input/tex/TexParser.js -> node_modules/mathjax-full/js/input/tex/ParseUtil.js -> node_modules/mathjax-full/js/input/tex/TexParser.js

This can lead to runtime errors as the bundler has to guess which module
to define first.

This commit removes this circular dependency by moving the functions in
ParseUtil that TexParser used into 2 new namespaces.

  • DimensionUtil -> matchDimen
  • StringUtil -> trimSpaces

I'm trying to be consistent with how the modules are laid out.

@zorkow
Copy link
Member

zorkow commented Sep 9, 2021

Thanks for the PR. The issue's been on the todo list for some time, so I appreciate you taking it on.
There are three changes I would like you to make before we can merge:

  • Rebase the PR onto the mathjax:develop branch (master branch always contains the latest stable version)
  • Move the content of ts/input/tex/StringUtil.ts into ts/util/string.ts.
  • Move the content of ts/input/tex/DimensionUtil.ts into ts/util/lengths.ts.

That should similarly take care of the circular dependency and will put all the utilities into the module where they actually should go. There might even be some scope for future code rationalisation in lengths.ts.

When bundling mathjax - you are warned that there is a circular
dependency:

```shell
code: CIRCULAR_DEPENDENCY, message: Circular dependency: node_modules/mathjax-full/js/input/tex/TexParser.js -> node_modules/mathjax-full/js/input/tex/ParseUtil.js -> node_modules/mathjax-full/js/input/tex/TexParser.js
```

This can lead to runtime errors as the bundler has to guess which module
to define first.

This commit removes this circular dependency.

> Note: there are still other circular dependencies in place that should
also be fixed:

```shell

1) components/loader.js > components/package.js
2) input/tex/mathtools/MathtoolsConfiguration.js > input/tex/mathtools/MathtoolsMappings.js > input/tex/mathtools/MathtoolsMethods.js > input/tex/mathtools/MathtoolsUtil.js
3) input/tex/mathtools/MathtoolsMethods.js > input/tex/mathtools/MathtoolsUtil.js
4) output/svg.js > output/svg/WrapperFactory.js > output/svg/Wrappers.js > output/svg/Wrapper.js

```
@edeustace edeustace changed the base branch from master to develop September 9, 2021 11:09
@edeustace
Copy link
Author

Hi @zorkow - I've applied the changes you requested - however - I've noted some other circular deps that should also be fixed. I'm a bit time pressed to do all the fixes right now. So we can merge and re-visit, or if you want to keep it all on the branch?

@bjesuiter
Copy link

bjesuiter commented Oct 28, 2021

Nice that you guys have already tackled the problem!
The Branch seems to be stable, can it be merged now?

I'm very interested in this and future fixes to this circular deps problem, since I'd like to write an Obsidian Plug-in, which can't compile on Rollup with marp-core (Markdown presentation engine), since it uses mathjax.

My issue is already linked above.

I'm also very time limited currently, but if there's something I can help with, I'll try to give it somewhat prio :)

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

3 participants