Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Use a different glob lib for now #11913

Merged
merged 3 commits into from
Feb 23, 2024

Conversation

bjester
Copy link
Member

@bjester bjester commented Feb 22, 2024

Summary

There are issues with yarn v1 and the node package glob >=10.0.0 because yarn v1's lack of support for package aliases and yarn's hoisting don't mix well

References

The following occurs indeterminately on Studio trying to use the latest kolibri-tools:

Error [ERR_REQUIRE_ESM]: require() of ES Module /home/bjester/Projects/learningequality/studio2/node_modules/wrap-ansi/index.js from /home/bjester/Projects/learningequality/studio2/node_modules/cliui/build/index.cjs not supported.
Instead change the require of index.js in /home/bjester/Projects/learningequality/studio2/node_modules/cliui/build/index.cjs to a dynamic import() which is available in all CommonJS modules.
    at Object.<anonymous> (/home/bjester/Projects/learningequality/studio2/node_modules/cliui/build/index.cjs:293:14)
    at Object.<anonymous> (/home/bjester/Projects/learningequality/studio2/node_modules/yargs/build/index.cjs:1:60678)
    at Object.<anonymous> (/home/bjester/Projects/learningequality/studio2/node_modules/yargs/index.cjs:5:30)
    at _yargs (/home/bjester/Projects/learningequality/studio2/node_modules/jest-cli/build/run.js:30:39)
    at buildArgv (/home/bjester/Projects/learningequality/studio2/node_modules/jest-cli/build/run.js:149:26)
    at Object.run (/home/bjester/Projects/learningequality/studio2/node_modules/jest-cli/build/run.js:124:24)
    at Object.<anonymous> (/home/bjester/Projects/learningequality/studio2/node_modules/jest-cli/bin/jest.js:16:17)

because glob requires jackspeak and in turn depends upon a forked version of yargs cliui that has dependency on wrap-ansi. The fork uses module aliases which we see here trying to unpackage wrap-ansi-cjs into an already existing wrap-ansi. Upgrading to a newer version of yarn should fix it but it also nets no improvement as there are still issues to resolve. We should prioritize it though, as well as upgrading kolibri-tools to ESM eventually. As far as the fork, there is an alternative PR open which would solve things without module aliases.

Reviewer guidance

yarn lint-frontend utilizes the modified code


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@github-actions github-actions bot added DEV: tools Internal tooling for development SIZE: medium labels Feb 22, 2024
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Code changes look good, manual testing of the lint-frontend yarn command checks out, and the compression logic successfully compressed all relevant files from examination of the WHL build logs.

@rtibbles rtibbles merged commit d7086d1 into learningequality:release-v0.16.x Feb 23, 2024
34 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
DEV: tools Internal tooling for development SIZE: medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants