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

fix: resolve lmdb binaries correctly #327

Merged
merged 2 commits into from
Mar 21, 2022
Merged

fix: resolve lmdb binaries correctly #327

merged 2 commits into from
Mar 21, 2022

Conversation

ascorbic
Copy link
Contributor

The fix for #294 did not cover all cases. Specifically, the lmdb module was resolving the incorrect path, so when using a version of Gatsby that uses it, we were trying to look in the wrong place for the binaries. We need to know their location because we copy them into the function bundle. This fix handles the case where lmdb resolves its root to the subdirectory, by looking a level up.

@netlify
Copy link

netlify bot commented Mar 18, 2022

Deploy Preview for netlify-plugin-gatsby-demo ready!

Name Link
🔨 Latest commit 4bd93d3
🔍 Latest deploy log https://app.netlify.com/sites/netlify-plugin-gatsby-demo/deploys/623842211e9e280009852188
😎 Deploy Preview https://deploy-preview-327--netlify-plugin-gatsby-demo.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Contributor

@orinokai orinokai left a comment

Choose a reason for hiding this comment

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

All looks good. Just worth mentioning that we could maybe dry up the code and make it more generic by replacing findModuleFromBase with something like the resolve-package-path module, but we might not want to introduce an external dependency when what we have already works in this case?

@ascorbic
Copy link
Contributor Author

All looks good. Just worth mentioning that we could maybe dry up the code and make it more generic by replacing findModuleFromBase with something like the resolve-package-path module, but we might not want to introduce an external dependency when what we have already works in this case?

True, that could be more robust against changes in future. We should do some tests at some point to see if it covers all of our cases.

@kodiakhq kodiakhq bot merged commit c6c162b into main Mar 21, 2022
@kodiakhq kodiakhq bot deleted the mk/resolve-lmdb branch March 21, 2022 09:26
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.

2 participants