-
-
Notifications
You must be signed in to change notification settings - Fork 9
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
fix: improve performance #14
Conversation
Replacing the regular expressions with algorithmic equivalents remediates performance issues. There are no doubt further performance enhancements that can be made to the code here, but this addresses the most critical. On my container, the performance tests added in this commit took less than 1 second to run with the code changes here compared to around 40 seconds using the current module code. (I also happen to think the algorithms are easier to understand, maintain, and debug in this form as well.)
@Trott this is great! I appreciate this, and I'm sure many in the community will as well. Thank you! Before I publish, I think we could make an argument for this being a patch release or a minor. What are your thoughts? edit: I think think it should be a patch, but would appreciate your perspective. |
IMO, it's a patch release. There are no (intended, at least) behavior changes other than improved performance and there are no new features being added to the API. |
done, published and released as patch version 4.0.2. thanks again! |
Awesome work! Thanks @Trott for implementing and @jonschlinkert for cutting a release! |
@jonschlinkert Is there any appetite for publishing a 2.x version? There are some possible changes that will simplify the code and (hopefully) make it more performant, but they will break on older versions of Node.js. Currently, Node.js itself supports 12.x, 14.x and 16.x, with 17.x coming in around a month or so. |
Ideally, we'd support node >=10.13 so glob-parent could upgrade as well. Was there something you were looking at that wouldn't exist in node 10? |
Oh, and I assumed this meant 5.x 🙈 |
Yes, absolutely. I was thinking about that last night. |
Replacing the regular expressions with algorithmic equivalents
remediates performance issues.
There are no doubt further performance enhancements that can be made
to the code here, but this addresses the most critical. On my
container, the performance tests added in this commit took less than 1
second to run with the code changes here compared to around 40 seconds
using the current module code.
(I also happen to think the algorithms are easier to understand,
maintain, and debug in this form as well.)