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 dependency on glob #435

Merged
merged 2 commits into from
Mar 10, 2024
Merged

Remove dependency on glob #435

merged 2 commits into from
Mar 10, 2024

Conversation

tomdracz
Copy link
Collaborator

@tomdracz tomdracz commented Mar 8, 2024

Summary

Closes #384 and supersedes #388

Ref thread here #388

Removes glob dependency that was only used in a single place - fetching entrypoints.

Rewrites getting entrypoint files using native fs module functions.

Tested running through test suite and veryfing the new function results in exactly same result as previous glob call

  • Add/update test to cover these changes
  • Update documentation
  • Update CHANGELOG file

@tomdracz tomdracz marked this pull request as ready for review March 8, 2024 20:27
@tomdracz tomdracz mentioned this pull request Mar 8, 2024
3 tasks
Copy link
Contributor

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

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

very nice! (and glad to see my completely untested pseudo code turned out to be pretty close!)

return []
})
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@G-Rath Is that the kind of thing you had in mind when talking about using readdirSync elsewhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup you've got it!

You could also check on Node 18+ to see if you get the same output with using recursive

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Verified recursive gives expected results when supported on Node 18+

Screenshot 2024-03-09 at 09 44 34

@tomdracz
Copy link
Collaborator Author

tomdracz commented Mar 8, 2024

very nice! (and glad to see my completely untested pseudo code turned out to be pretty close!)

Indeed! Took me couple tries with Copilot to get this working but yeah, was pretty close from the get go!

@tomdracz tomdracz merged commit 3be8b49 into next Mar 10, 2024
27 checks passed
@tomdracz tomdracz deleted the remove-glob-dependency branch March 10, 2024 10:01
ahangarha pushed a commit that referenced this pull request Mar 28, 2024
* Remove dependency on glob

* Changelog
justin808 added a commit that referenced this pull request May 8, 2024
* Remove deprecated webpacker spelling (#429)
* feat!: drop support for Ruby 2.6 (#415)
* feat!: remove `https` option (#414)
* feat!: remove `relative_url_root` option (#413)
* feat!: drop support for Node v12 (#431)
* feat!: remove `yarn_install` task (#412)
* ci: cancel in-progress jobs when new changes are pushed (#432)
* ci: merge ruby and node based jobs into single workflows (#434)
* ci: merge node related checks into a single workflow
* ci: merge ruby related checks into a single workflow
* ci: rename jobs
* ci: test against Ruby 3.1 and 3.2, and Node 20 (#433)
* ci: test against Ruby 3.1 and 3.2
* ci: test against Node 20
* ci: only run Rubocop with latest Ruby (#438)
* chore: update pull request template (#437)
* ci: test against Rails 7.1 (#436)
* feat!: remove `globalMutableWebpackConfig` (#439)
* ci: don't run on pushes to `next` (#444)
* feat!: remove `verify_file_existance` method (#446)
* feat!: remove `check_yarn` task (#443)
* ci: use latest bundler in generator jobs (#445)
* feat!: enable `ensure_consistent_versioning` by default (#447)
* Remove dependency on glob (#435)
* Remove dependency on glob
* Pass in shakapacker config consistently (#448)
* Reference config from config rather than instance in compiler
* Strip additional_paths from the asset paths generated in the file.js rule (#403)
* Add config.includePaths and strip includePaths from file.js output paths
* Refactor code to fix lint issue
* Make sure to replace only once
* Change variable name
* Revert includePath change and make stripping additional_paths opt-in
* refactor: use snake_case for method name (#450)
* fix: only strip paths that start with source path (#451)
* feat!: always use `package_json` for managing node packages (#430)
* feat!: minor cleanup of js utilities (#454)
* test: require `ostruct` explicitly (#460)
* ci: only run ESLint on latest Node (#462)
* docs: create upgrade guide for v8 (#458)
* V8 upgrade guide
* docs: write v8 upgrade doc
* docs: format with Prettier
* feat!: move tests and helpers into  (#459)
* test: move into `test` directory
* test: update import paths
* test: update jest config
* refactor: move test helpers
* test: move jest config into dedicated file
* docs: add changelog entry
* chore: remove unneeded eslint ignore
* refactor: update js linting (#461)
* refactor: setup and apply Prettier (#463)
* refactor: use `eslint-plugin-jest` (#464)
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.

2 participants