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

More compatible dependency discovery #11

Merged
merged 5 commits into from
Jul 30, 2020

Conversation

toddhgardner
Copy link
Member

Fixes #9 by changing the approach to discovering dependencies. Instead of getting a listing from npm, which requires access to the shell, we can look for package.json files in the current directories that have been cached by the NodeJS module system.

Upon the first (and only the first) installation of the agent, dependencies are discovered by pulling a list of cached modules and recursing up the folder structure to find all the package.json files. The logic limits recursion to 10 levels for sanity, and does not look at the same directory twice.

I don't believe we'll have permission problems, because the running process would already need access to these files in order to load them.

Testing this required a standalone test for loading up and caching modules, which appears to be intercepted in the normal Jest unit testing structure.


I also removed a test impacted by #10.

@BrandesEric
Copy link
Member

Cool! Quick question - does this new mechanism to retrieve deps just get "top-level" dependencies listed in package.json? Or does it grab peer/child dependencies as well?

@toddhgardner
Copy link
Member Author

Neither, actually. It grabs every package that has been require'd (or imported) and discovers its package.json version. So it won't list downstream dependencies, and it might not list top-level either if they haven't been used.

Subtle change from before, as before it listed all top level and global, regardless if they were used. But I think this is more relevant, actually.

@BrandesEric
Copy link
Member

Cool cool, that does seem useful!

@BrandesEric
Copy link
Member

Sorry, one last question - if an application installs TrackJS early on in the app startup process, maybe before they require a bunch of other modules, do we run any risk of the cached deps list being much shorter than once the app is working full swing?

@toddhgardner
Copy link
Member Author

Hmm, that's an interesting question. If a user installed the agent, then went on to load more modules afterwards, they would not be in the module cache, and we wouldn't get them.

I think we should move this to build the dependencies when the first error is recorded, rather than during install. This used to be an issue when discovering dependencies was async, but now that it's sync, it should be easy to move.

Real good point! Will fix.

@BrandesEric
Copy link
Member

I like that idea! Nice to defer the file system traversal until necessary too!

@J-Griffin
Copy link

Cool. I like the dir access stuff vs shelling out. Definitely seems like a better plan as far as compatibility goes.

@toddhgardner toddhgardner merged commit 93c5a1a into master Jul 30, 2020
@toddhgardner toddhgardner deleted the more-compatible-dependency-discovery branch July 30, 2020 14:05
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.

Faults recorded during dependency discovery in secure environments.
3 participants