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

WIP - Update glob to v10 #388

Closed
wants to merge 2 commits into from
Closed

Conversation

kiskoza
Copy link

@kiskoza kiskoza commented Dec 19, 2023

Summary

I've spotted #384 and I thought I give it a try. Shakapacker currently depends on an older version of glob which has a dependency with known memory leak. The author of glob ditched that dependency aleady and made "Massive performance improvements." in v9. In v10 the only breaking change is that there are no default exports, but Shakapacker is not using that anyways, so I thought it's better to use the latest, maintained version of the package instead of pinning it to v9.

Though it seems to be an easy upgrade, it has some possible drawbacks: the new glob versions requires at least nodejs v16. Shakapacker currently supports ^12.13.0 || ^14 || >=16. The official support for v12 ended more than a year ago, for v14 it ended 7 months ago (reference), so I don't think it should cause a problem.

Pull Request checklist

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

Other Information

Glob in dev dependencies

There are some dev dependencies that still require glob v7, but those shouldn't affect shakapacker users. As far as I read about these packages, they are going to upgrade glob as soon as they can drop nodejs 14, for example jestjs/jest#14509

$> yarn why glob
yarn why v1.22.19
[1/4] Why do we have the module "glob"...?
[2/4] Initialising dependency graph...
[3/4] Finding dependency...
[4/4] Calculating file sizes...
=> Found "glob@10.3.10"
info Has been hoisted to "glob"
info This module exists because it's specified in "dependencies".
info Disk size without dependencies: "1.21MB"
info Disk size with unique dependencies: "3.25MB"
info Disk size with transitive dependencies: "4.2MB"
info Number of shared dependencies: 21
=> Found "@jest/reporters#glob@7.2.0"
info This module exists because "jest#@jest#core#@jest#reporters" depends on it.
info Disk size without dependencies: "64KB"
info Disk size with unique dependencies: "204KB"
info Disk size with transitive dependencies: "300KB"
info Number of shared dependencies: 8
=> Found "jest-config#glob@7.2.0"
info This module exists because "jest#@jest#core#jest-config" depends on it.
info Disk size without dependencies: "64KB"
info Disk size with unique dependencies: "204KB"
info Disk size with transitive dependencies: "300KB"
info Number of shared dependencies: 8
=> Found "jest-runtime#glob@7.2.0"
info This module exists because "jest#@jest#core#jest-runtime" depends on it.
info Disk size without dependencies: "64KB"
info Disk size with unique dependencies: "204KB"
info Disk size with transitive dependencies: "300KB"
info Number of shared dependencies: 8
=> Found "rimraf#glob@7.2.0"
info This module exists because "jest#@jest#core#rimraf" depends on it.
info Disk size without dependencies: "64KB"
info Disk size with unique dependencies: "204KB"
info Disk size with transitive dependencies: "300KB"
info Number of shared dependencies: 8
=> Found "test-exclude#glob@7.2.0"
info This module exists because "jest#@jest#core#@jest#transform#babel-plugin-istanbul#test-exclude" depends on it.
info Disk size without dependencies: "64KB"
info Disk size with unique dependencies: "204KB"
info Disk size with transitive dependencies: "300KB"
info Number of shared dependencies: 8

@ahangarha ahangarha changed the title Update glob to v10 WIP - Update glob to v10 Dec 20, 2023
@justin808
Copy link
Member

@kiskoza can you solve the JS test failures?

@kiskoza
Copy link
Author

kiskoza commented Dec 21, 2023

The issue with the CI was that I removed nodejs v14 from the supported engine versions, but the CI was configured to use that in the matrix. I removed v14 from the CI matrix.

@justin808
Copy link
Member

This sounds good, but do we need to update the major version of shakapacker to require node 16+?

CC: @G-Rath

@G-Rath
Copy link
Contributor

G-Rath commented Dec 22, 2023

It looks like this is the only use of glob:

const rootPath = join(config.source_path, config.source_entry_path)
if (config.source_entry_path === '/' && config.nested_entries) {
throw new Error(
"Your shakapacker config specified using a source_entry_path of '/' with 'nested_entries' == " +
"'true'. Doing this would result in packs for every one of your source files"
)
}
const nesting = config.nested_entries ? '**/' : ''
globSync(`${rootPath}/${nesting}*.*`).forEach((path) => {

Technically this includes user input via source_path and source_entry_path on the first line of that snippet which if it included a glob would be treated as such but I can't find any evidence of those ever expecting to have a glob and I would expect other things would break if you actually tried to use a glob in them.

As such, I think we could just remove glob entirely in favor of a recursive fs.readdirSync along the lines of:

const getSourceFiles = (rootPath, includeNested) => {
  if(!includeNested) {
    return fs.readdirSync(rootPath);
  }

  return fs.readdirSync(rootPath, { withFileTypes: true }).flatMap(dirent => {
    if (dirent.isDirectory()) {
      return getSourceFiles(join(rootPath, dirnet.name), true);
    }

    return dirnet.name;
  })
}

(that's very rough since I didn't want to steal @kiskoza's thunder, though I am happy to pick this up if folks want 🙂)

That would probably be a bit slower than glob but that shouldn't matter too much because this is only called a couple of times (maybe only once?) when compiling (+ I don't expect it to be that horrible as glob has to be something similar and its smarts are really about very complex globs where caching etc are useful), and Node v18/v20 added recursive so eventually we'd be able to greatly simplify that too.

That should result in addressing #384 without requiring dropping any node versions or a new major version (though @justin808 since we're expecting to do a new major sometime in next few months, I plan to talk about dropping support for Ruby 2.x and so it still might be worth dropping support for older versions of Node too, but we can talk about that separately)

@justin808
Copy link
Member

The Node code is used only at build time, so I doubt a memory leak would affect much.

I'd say we defer until v8 and drop support for older Node.

@justin808 justin808 added this to the v8 milestone Dec 29, 2023
@G-Rath
Copy link
Contributor

G-Rath commented Dec 29, 2023

I think that's fine on the short-term but long-term it still seems like a win to drop a dependency entirely so if anyone wants to pick that up I'm happy to review it - otherwise I'll pick it up sometime either after v8 or if it starts getting flagged by osv-scanner and dependabot

@ahangarha
Copy link
Contributor

Bundler has dropped support for Ruby 2.6 and 2.7 in their recent 2.5.0 release. Since Bundler is a far more prominent package than Shakapacker, I think we can conclude from their practices that we can drop support for Node 14 without making a major release.

But also I think replacing glob with fs.readdirSync is a good option, if not a better one.

@G-Rath
Copy link
Contributor

G-Rath commented Dec 29, 2023

I think we can conclude from their practices that we can drop support for Node 14 without making a major release.

I can't find anything about their versioning that indicates why they didn't consider dropping the Ruby versions a breaking change but under traditional semver (which they might not be following) it is a breaking change, but I can say in the Node community at least dropping support for a version of Node is considered a breaking change - there can be some wiggle room such as if the versions are super old and a package doesn't have an explicit "engines" field, but overall I don't think this change is critical enough to be worth dropping that support especially when we're planning to do a new major soon anyway and we agree that the fs.readdirSync is the better path 🤷

@kiskoza
Copy link
Author

kiskoza commented Jan 2, 2024

Thanks for all your input on the topic. It feels like we shouldn't make this change yet as dropping support for older nodejs versions require a major version bump in the npm package, but this dependency is hardly used and the change is not big enough to be the reason to drop older nodejs, the original issue could be solved in other ways as well.

I'm happy to either leave it open or just close it as it's already referenced from the issue and could be reopened later when we need it

@ahangarha
Copy link
Contributor

Is there any argument against switching to fs.readdirSync?

@tomdracz tomdracz mentioned this pull request Mar 8, 2024
3 tasks
@tomdracz
Copy link
Collaborator

tomdracz commented Mar 8, 2024

Been no movement on this one so opened my own stab for this at #435

@tomdracz tomdracz closed this Mar 10, 2024
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.

5 participants