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

Perf fix: cache the resolutions identically to how require does. #97

Merged
merged 1 commit into from
Feb 28, 2019

Conversation

stefanpenner
Copy link
Collaborator

@stefanpenner stefanpenner commented Feb 26, 2019

Turns out this is extremely expensive, and apps with many
apps/engines/in-repo addons spend up to minutes here.

We should discuss better/improved/ideal solutions, i don't like this approach really, and we keep playing wack-a-mole with similar solutions anytime we use require.resolve....

But this is worth a discussion, as this approach really does save ALOT (for very large apps, minutes) of startup time.

@rwjblue
Copy link
Member

rwjblue commented Feb 26, 2019

Current CI failure is due to eslint failures.

src/npm-dependency-version-checker.js Outdated Show resolved Hide resolved
src/npm-dependency-version-checker.js Outdated Show resolved Hide resolved
@stefanpenner
Copy link
Collaborator Author

stefanpenner commented Feb 26, 2019

Alright back from lunch, going to tidy this up.

@stefanpenner stefanpenner changed the title POC Perf fix: cache the resolutions identically to how require does. Perf fix: cache the resolutions identically to how require does. Feb 27, 2019
@stefanpenner
Copy link
Collaborator Author

Alright, created resolve-package-path which harvests @davecombs's work from hash-for-dep into a reusable library. We can then have this and that library both share the same code/cache.

Turns out this is extremely expensive, and apps with many
apps/engines/in-repo addons spend up to minutes here.
@rwjblue rwjblue merged commit d24bca1 into master Feb 28, 2019
@delete-merged-branch delete-merged-branch bot deleted the perf-poc branch February 28, 2019 21:38
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