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

Enable the import/no-cycle ESLint plugin rule #16515

Merged
merged 1 commit into from
Jun 4, 2023

Conversation

Snuffleupagus
Copy link
Collaborator

Having cyclical imports is obviously not a good idea, and this ESLint plugin rule can help detect those; please see https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/no-cycle.md

@Snuffleupagus
Copy link
Collaborator Author

@calixteman Do you have any ideas/opinions on how to best break the dependency cycles in the xfa/-code, without having to move loads of code?
See https://github.com/mozilla/pdf.js/actions/runs/5155612716/jobs/9285509966?pr=16515#step:6:54

@calixteman
Copy link
Contributor

We could move the symbols $... used in som.js into a separate file which can be then imported by both files.
Then we can add a flag in both classes XFAObject and XFAObjectArray (e.g. isXfaObject and isXFAObjectArray) to avoid the use of instanceof in som.js.
And to fix the XmlObject issue, since createNodes is not really related to the som stuff we could move it in XFAObject as a method and then in som.js replace the use of createNodes(root, path) by root.createNodes(path).

@Snuffleupagus Snuffleupagus force-pushed the eslint-import-no-cycle branch 4 times, most recently from 4879b2f to 4f83454 Compare June 3, 2023 10:22
@mozilla mozilla deleted a comment from moz-tools-bot Jun 3, 2023
@mozilla mozilla deleted a comment from moz-tools-bot Jun 3, 2023
@mozilla mozilla deleted a comment from moz-tools-bot Jun 3, 2023
@mozilla mozilla deleted a comment from moz-tools-bot Jun 3, 2023
@mozilla mozilla deleted a comment from moz-tools-bot Jun 3, 2023
@Snuffleupagus Snuffleupagus force-pushed the eslint-import-no-cycle branch from 4f83454 to db930f5 Compare June 3, 2023 10:40
@mozilla mozilla deleted a comment from moz-tools-bot Jun 3, 2023
@mozilla mozilla deleted a comment from moz-tools-bot Jun 3, 2023
@mozilla mozilla deleted a comment from moz-tools-bot Jun 3, 2023
@Snuffleupagus
Copy link
Collaborator Author

/botio xfatest

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_xfatest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.193.163.58:8877/962eb8037796cb0/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_xfatest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/7088a1a2e2ed2dc/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/7088a1a2e2ed2dc/output.txt

Total script time: 9.23 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 2

Image differences available at: http://54.241.84.105:8877/7088a1a2e2ed2dc/reftest-analyzer.html#web=eq.log

@Snuffleupagus Snuffleupagus marked this pull request as ready for review June 3, 2023 10:56
@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/962eb8037796cb0/output.txt

Total script time: 18.06 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 2

Image differences available at: http://54.193.163.58:8877/962eb8037796cb0/reftest-analyzer.html#web=eq.log

Copy link
Contributor

@timvandermeij timvandermeij left a comment

Choose a reason for hiding this comment

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

Looks good to me, with a rebase. Thanks!

Having cyclical imports is obviously not a good idea, and this ESLint plugin rule can help detect those; please see https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/no-cycle.md
@Snuffleupagus Snuffleupagus force-pushed the eslint-import-no-cycle branch from db930f5 to cf3a35e Compare June 4, 2023 11:44
@Snuffleupagus Snuffleupagus merged commit a5d0af3 into mozilla:master Jun 4, 2023
@Snuffleupagus Snuffleupagus deleted the eslint-import-no-cycle branch June 4, 2023 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants