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

Delay connect require until authenticate called #17

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Yukaii
Copy link

@Yukaii Yukaii commented Mar 18, 2019

In d53ff7d (d53ff7d#diff-3d4f4aac823ef6b0561ea61cb312b2c7R10), it moves const connect = require('../framework/connect') to the top. But it brings up the following error for me because there's a circular require in both authentication.js and connect.js.

TypeError: connect.__monkeypatchNode is not a function
    at authenticate (node_modules/@passport-next/passport/lib/middleware/authenticate.js:101:15)
    at Layer.handle [as handle_request] (node_modules/express/lib/router/layer.js:95:5)
    at trim_prefix (node_modules/express/lib/router/index.js:317:13)
    at node_modules/express/lib/router/index.js:284:7

This fix reverts part of commit d53ff7d, and moves require connect to original position.

Checklist

  • I have read the CONTRIBUTING guidelines.
  • I have added test cases which verify the correct operation of this feature or patch.
  • I have added documentation pertaining to this feature or patch.
  • The automated test suite ($ make test) executes successfully.
  • The automated code linting ($ npm run-script lint) executes successfully.

@adamhathcock
Copy link

Would it be better to refactor the code in connect.js to not require the inline require?

@rwky
Copy link

rwky commented Mar 18, 2019

@Yukaii can you please provide sample code that triggers this error? The test suite doesn't trigger it and it's not happening for me in production either.

@rwky rwky requested review from rwky and a team and removed request for idurotola March 18, 2019 10:36
@Yukaii
Copy link
Author

Yukaii commented Mar 18, 2019

@rwky Ok, I'll try reproduce in a minimal repository in the next few days :)

Providing more context, I use passport-next with passport-mock-strategy. I'll try to find out where the problem is.

@rwky
Copy link

rwky commented Apr 24, 2019

@Yukaii had a chance to create the test repo?

@Yukaii
Copy link
Author

Yukaii commented Apr 25, 2019

@rwky Didn't have a time to do this yet 😓 Might look into this on weekends.

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