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

remove array-flatten dependency, no longer necessary #5219

Closed

Conversation

benjaminPla
Copy link

replaced array-flatten dependency with the new flat() method from JavaScript, which is supported in Node.js version 11.0.0 and above. This simplifies the code and reduces the number of dependencies

replaced array-flatten dependency with the new flat() method from JavaScript, which is supported in Node.js version 11.0.0 and above. This simplifies the code and reduces the number of dependencies. Proposed change for Express.js v5.
@@ -211,7 +210,7 @@ app.use = function use(fn) {
}
}

var fns = flatten(slice.call(arguments, offset));
var fns = slice.call(arguments, offset).flat()
Copy link
Member

Choose a reason for hiding this comment

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

You need flat(Infinity) for the same behavior.

Copy link
Member

Choose a reason for hiding this comment

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

Keep in mind that changing this is a breaking change, so you'd want to be making the change in Express.js v5 and not to the current v4 release.

Copy link
Author

Choose a reason for hiding this comment

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

Indeed, based on the test results, these changes are intended for a future version of Express.

Sorry for wanting to remove your dependency! 🤣

Choose a reason for hiding this comment

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

Now that V5 is out, could this PR be revisited?

Copy link
Member

Choose a reason for hiding this comment

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

A similar change was merged in #5677 which is in v5.

@blakeembrey
Copy link
Member

Closing as #5677 was merged.

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.

4 participants