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 Node module loading #445

Merged
merged 1 commit into from
Dec 7, 2016

Conversation

mdebruijne
Copy link
Contributor

This patch fixes module loading on Node.js.

@davidlehn
Copy link
Member

What are the non functions that leak through to that point?
Looks like this change won't break other usage. Anyone know if there are edge cases to be aware of?

@mdebruijne
Copy link
Contributor Author

Considering the plans in #357 I haven't spent time in analyzing/optimizing further. This fix eliminates the unexpected throw and therefore prevents crashes in applications/libraries that depend on forge. In my case Google Cloud Node.js Client, Google APIs Node.js Client and Jest.

@dlongley
Copy link
Member

dlongley commented Dec 1, 2016

I think this is fine to merge. I'm only concerned that it's covering up a problem or not fully solving it in those environments where it seems to make a difference. That being said, we're overhauling the module loading system in a new version, so I think this is a perfectly fine stop gap measure if it helps people. We should merge on those grounds, IMO.

@pie6k
Copy link

pie6k commented Dec 5, 2016

I've lost 2 hours looking for this. (Happened when I was trying to import 'parse-server' when testing with jest

@mdebruijne
Copy link
Contributor Author

@davidlehn , @dlongley

Has consensus been reached that this pull request can be merged?

The current version of Forge is causing crashes in many places with corresponding upstream issue reports.

Please let me know if I can do anything to help moving this forward. Your support in merging this pull request and releasing an update will be very much appreciated by anyone who (indirectly) depend on Forge.

@davidlehn davidlehn merged commit 054bf71 into digitalbazaar:master Dec 7, 2016
@davidlehn
Copy link
Member

davidlehn commented Dec 7, 2016

I merged this since it hopefully won't cause other problems. But I was unable to reproduce the issue with basic require tests. Can someone please post a test case is that fails? Also since I can't test the issue I would still like to know an answer my previous question about what non-function data is that is leaking through. If someone could console.log it that would be nice. It would be better to fix the cause of this problem rather than hack around it.

EDIT: discuss in #362

@davidlehn
Copy link
Member

Released as 0.6.46.

@mdebruijne
Copy link
Contributor Author

Thanks!

LegNeato added a commit to LegNeato/google-p12-pem that referenced this pull request Dec 8, 2016
This picks up the fix for digitalbazaar/forge#445, which makes this library not throw under `jest` automocked tests.
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.

4 participants