Skip to content

fix: should only merge plain objects#9675

Closed
g1eny0ung wants to merge 1 commit into
nextauthjs:mainfrom
g1eny0ung:merge-plain-objects
Closed

fix: should only merge plain objects#9675
g1eny0ung wants to merge 1 commit into
nextauthjs:mainfrom
g1eny0ung:merge-plain-objects

Conversation

@g1eny0ung

@g1eny0ung g1eny0ung commented Jan 18, 2024

Copy link
Copy Markdown

☕️ Reasoning

Refs: #2509, #3944.

I got the info from #3944 (reply in thread), which points to the merge function as the reason for the loss of some constructed object types, for example, the Agent type (new HttpsProxyAgent()) will be wrongly overwritten as object.

This stack overflow answer doesn't consider more boundary cases, we should let it only merge pure objects and let constructed objects or the native objects overwrite directly, thus preserving its original type.

So in this PR, I use a tool library is-plain-obj to replace the existing isObject function to resolve this problem. I manually tested with:

GithubProvider({
  clientId: process.env.GITHUB_ID as string,
  clientSecret: process.env.GITHUB_SECRET as string,
  httpOptions: {
    timeout: 10000, // Increase timeout to 10 seconds.
    ...(process.env.http_proxy && {
      // Use a proxy agent if a proxy is defined.
      agent: new HttpsProxyAgent(process.env.http_proxy as string),
    }),
  },
})

in my Next.js project, and seems it works.

🧢 Checklist

  • Documentation
  • Tests
  • Ready to be merged

🎫 Affected issues

📌 Resources

@vercel

vercel Bot commented Jan 18, 2024

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
auth-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 18, 2024 5:10pm
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
next-auth-docs ⬜️ Ignored (Inspect) Visit Preview Jan 18, 2024 5:10pm

@OffBy0x01

Copy link
Copy Markdown

I'll try this out today or Monday. Really excited to see if it works for us - this is by far our biggest pain point with nextauth.

@g1eny0ung

Copy link
Copy Markdown
Author

@ThangHuuVu @balazsorban44 Can you guys approve to get workflows running? I've only run some of the tests locally for network reasons, so I'd like to see the results of the CI run.

@balazsorban44

Copy link
Copy Markdown
Contributor

Hi, this is likely not going to be what you intended. The new core doesn't use openid-client, hence it doesn't have the httpOptions either. We switched to oauth4webapi.

We will revisit this at a later point. You can follow #8491 until then.

@g1eny0ung

g1eny0ung commented Jan 24, 2024

Copy link
Copy Markdown
Author

@balazsorban44 But version v4 should be considered a current stable release, shouldn't we be working on bug fixes? Looking at the release and documentation updates, I don't see the new core being released anytime soon. The tutorial of Next.js OAuth is still using the next-auth package (https://authjs.dev/getting-started/providers/oauth-tutorial).

And PR #8491 only indicates the absence of httpOptions in the new core client implementation, I don't see any correlation with this PR?

But even if I wanted to fix this on the main branch, I understand that maybe your team wants to focus on the big new prerelease. I'll make my patch file available here for others to look at, and for now, using the pnpm patch approach has helped me resolve the problem. Compared to the old approach, it is now possible to add the httpOptions.agent directly to the project file for better static analysis. That is, this approach (also means this PR) goes to the source of the problem.

@g1eny0ung

g1eny0ung commented Jan 24, 2024

Copy link
Copy Markdown
Author

Below is the patched utils/merge.js, I just copied the main function in https://github.com/sindresorhus/is-plain-obj/blob/main/index.js to avoid install the package:

Commands

pnpm patch next-auth
# After the modification
pnpm patch-commit <path>

merge.js

"use strict";

Object.defineProperty(exports, "__esModule", {
  value: true
});
exports.merge = merge;

function isPlainObject(value) {
	if (typeof value !== 'object' || value === null) {
		return false;
	}

	const prototype = Object.getPrototypeOf(value);
	return (prototype === null || prototype === Object.prototype || Object.getPrototypeOf(prototype) === null) && !(Symbol.toStringTag in value) && !(Symbol.iterator in value);
}

function merge(target, ...sources) {
  if (!sources.length) return target;
  const source = sources.shift();

  if (isPlainObject(target) && isPlainObject(source)) {
    for (const key in source) {
      if (isPlainObject(source[key])) {
        if (!target[key]) Object.assign(target, {
          [key]: {}
        });
        merge(target[key], source[key]);
      } else {
        target[key] = source[key];
      }
    }
  }

  return merge(target, ...sources);
}

@flash4174

Copy link
Copy Markdown

Hi All,

I get the error 'this.createSocket is not a function'

Any idea? I've tried both client.js and merge.js edit and also both the same time

@g1eny0ung

Copy link
Copy Markdown
Author

Hi All,

I get the error 'this.createSocket is not a function'

Any idea? I've tried both client.js and merge.js edit and also both the same time

🤔 No ideas. I used "next-auth": "^4.24.5" and now it works as normal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Refers to `@auth/core`

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants