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 lodash dependency #2421

Merged
merged 4 commits into from
Sep 13, 2023
Merged

Conversation

bartocc
Copy link
Contributor

@bartocc bartocc commented Jun 2, 2022

I might be wrong, but this is the only place in the codebase where we use a lodash function.

lodash is quite big, and even if ember-cli-mirage is only bundled in dev/test builds, this could be refactored with typeof and we could completely drop the lodash dependency…

Copy link
Collaborator

@SergeAstapov SergeAstapov 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

@bartocc bartocc changed the title Remove-lodash-dep ➖ Remove lodash dependency Jun 2, 2022
@cah-brian-gantzler
Copy link
Collaborator

Prettier gets me every time. Want to add lint-staged to the git commit hook to run it for me 😄

@bartocc
Copy link
Contributor Author

bartocc commented Jun 2, 2022

@SergeAstapov I've ran yarn workspace ember-cli-mirage-docs test:ember locally and it passes. How can I fix the CI /Docs action?

@SergeAstapov
Copy link
Collaborator

@bartocc the stack trace makes me think the issue caused by some transitive dependency that got drifted:

ERROR Summary:

  - broccoliBuilderErrorStack: TypeError: Cannot read property 'program' of undefined
    at DocFactory._inspectExportDefaultDeclaration (/home/runner/work/ember-cli-mirage/ember-cli-mirage/node_modules/esdoc/out/src/Factory/DocFactory.js:136:40)
    at new DocFactory (/home/runner/work/ember-cli-mirage/ember-cli-mirage/node_modules/esdoc/out/src/Factory/DocFactory.js:91:10)
    at Function._traverse (/home/runner/work/ember-cli-mirage/ember-cli-mirage/node_modules/esdoc/out/src/ESDoc.js:232:21)
    at /home/runner/work/ember-cli-mirage/ember-cli-mirage/node_modules/esdoc/out/src/ESDoc.js:114:25
    at Function._walk (/home/runner/work/ember-cli-mirage/ember-cli-mirage/node_modules/esdoc/out/src/ESDoc.js:203:9)
    at Function._walk (/home/runner/work/ember-cli-mirage/ember-cli-mirage/node_modules/esdoc/out/src/ESDoc.js:205:14)
    at Function.generate (/home/runner/work/ember-cli-mirage/ember-cli-mirage/node_modules/esdoc/out/src/ESDoc.js:98:10)
    at generateESDoc (/home/runner/work/ember-cli-mirage/ember-cli-mirage/node_modules/ember-cli-addon-docs-esdoc/lib/preprocessors/generate-esdoc.js:53:11)
    at generateESDocJsonApi (/home/runner/work/ember-cli-mirage/ember-cli-mirage/node_modules/ember-cli-addon-docs-esdoc/lib/preprocessors/generate-esdoc-jsonapi.js:67:14)
    at /home/runner/work/ember-cli-mirage/ember-cli-mirage/node_modules/ember-cli-addon-docs-esdoc/lib/broccoli/generator.js:41:23

however <=3.24 scenarios fail with different error

Uncaught Error: Could not find module `@glimmer/manager` imported from `(require)`

I just saw that exact same error in https://github.com/miguelcobain/ember-css-transitions/runs/6711631246?check_suite_focus=true which makes me think it's again comes from transitive dependency.

I don't have time right to look into root cause, will try to do once I have a chance.

@bartocc
Copy link
Contributor Author

bartocc commented Jun 20, 2022

@SergeAstapov any idea how I could help debug the CI?

@SergeAstapov SergeAstapov marked this pull request as ready for review September 13, 2023 01:33
@SergeAstapov SergeAstapov merged commit 24af2b5 into miragejs:master Sep 13, 2023
12 checks passed
@SergeAstapov
Copy link
Collaborator

Thank you @bartocc!

@bartocc bartocc deleted the remove-lodash-dep branch September 13, 2023 07:22
@bartocc bartocc restored the remove-lodash-dep branch September 13, 2023 07:22
francois2metz pushed a commit to francois2metz/ember-cli-mirage that referenced this pull request Dec 19, 2023
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.

3 participants