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 unused originalIndex from acceptParams #5119

Conversation

raksbisht
Copy link
Contributor

Remove unused originalIndex from acceptParams in express/lib/utils.js

@dougwilson
Copy link
Contributor

Hello, and thank you for your pull request. Unfortunately even though Express.js itself may not use this param, external code commonly uses the functils in our utils, so the API for them is frozen due to external usage. I hope that makes sense.

@dougwilson dougwilson closed this Feb 8, 2023
@dougwilson dougwilson added the pr label Feb 8, 2023
@dougwilson
Copy link
Contributor

Apologies, acceptParams is not an exported function I think.

@dougwilson dougwilson reopened this Feb 8, 2023
@raksbisht
Copy link
Contributor Author

Thanks @dougwilson!

lib/utils.js Outdated Show resolved Hide resolved
Copy link
Member

@LinusU LinusU left a comment

Choose a reason for hiding this comment

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

From my analysis, the only way to observe originalIndex from outside of Express is if you are doing a deep require into the utils file, and are calling normalizeType directly. Even so, originalIndex is always undefined, so it's only observable if doing 'originalIndex' in result.

Since we don't support deep requires into internals, and since even if someone did this it's very unlikely to affect anyone, I think that we can merge this 👍


const utils = require('express/lib/utils')

'originalIndex' in utils.normalizeType('foo/bar')

The code above previously evaluated to true, but will become false with this patch.

@dougwilson
Copy link
Contributor

Since we don't support deep requires into internals

Annoyingly we do in 4.x, but it doesn't matter, as like you observed, the functionality doesn't actually "produce" anything -- a property that is defined on the object in a technical sense, but extremely unlikely to actually break anything with it's removal 👍

@LinusU
Copy link
Member

LinusU commented Feb 21, 2023

Annoyingly we do in 4.x

Whoops, my bad. Good to know 👍

@dougwilson dougwilson force-pushed the fixes/remove-unused-originalindex-from-acceptParams branch from 84fe482 to a1efd9d Compare February 22, 2023 03:55
@dougwilson dougwilson merged commit a1efd9d into expressjs:master Feb 22, 2023
neohotsauce pushed a commit to neohotsauce/express that referenced this pull request May 4, 2023
@UlisesGascon UlisesGascon mentioned this pull request Feb 26, 2024
github-actions bot pushed a commit to brafdlog/caspion that referenced this pull request Apr 2, 2024
# [1.26.0](v1.25.2...v1.26.0) (2024-04-02)

### Upgrade

* Bump express from 4.18.2 to 4.19.2 (#552) ([bd280ba](bd280ba)), closes [#552](#552) [expressjs/express#5552](expressjs/express#5552) [expressjs/express#5556](expressjs/express#5556) [expressjs/express#5527](expressjs/express#5527) [expressjs/express#5511](expressjs/express#5511) [expressjs/express#5510](expressjs/express#5510) [expressjs/express#5541](expressjs/express#5541) [expressjs/express#5551](expressjs/express#5551) [expressjs/express#5541](expressjs/express#5541) [expressjs/express#5032](expressjs/express#5032) [expressjs/express#5034](expressjs/express#5034) [expressjs/express#5027](expressjs/express#5027) [expressjs/express#5124](expressjs/express#5124) [expressjs/express#5119](expressjs/express#5119) [expressjs/express#5117](expressjs/express#5117) [expressjs/express#5113](expressjs/express#5113) [expressjs/express#5130](expressjs/express#5130) [expressjs/express#5131](expressjs/express#5131) [expressjs/express#5028](expressjs/express#5028) [expressjs/express#5137](expressjs/express#5137) [#5541](https://github.com/brafdlog/caspion/issues/5541)
* Bump express from 4.19.2 in /ui-react (#551) ([8d6032d](8d6032d)), closes [#551](#551) [expressjs/express#5552](expressjs/express#5552) [expressjs/express#5556](expressjs/express#5556) [expressjs/express#5527](expressjs/express#5527) [expressjs/express#5511](expressjs/express#5511) [expressjs/express#5510](expressjs/express#5510) [expressjs/express#5541](expressjs/express#5541) [expressjs/express#5551](expressjs/express#5551) [expressjs/express#5541](expressjs/express#5541) [expressjs/express#5032](expressjs/express#5032) [expressjs/express#5034](expressjs/express#5034) [expressjs/express#5027](expressjs/express#5027) [expressjs/express#5124](expressjs/express#5124) [expressjs/express#5119](expressjs/express#5119) [expressjs/express#5117](expressjs/express#5117) [expressjs/express#5113](expressjs/express#5113) [expressjs/express#5130](expressjs/express#5130) [expressjs/express#5131](expressjs/express#5131) [expressjs/express#5028](expressjs/express#5028) [expressjs/express#5137](expressjs/express#5137) [#5541](https://github.com/brafdlog/caspion/issues/5541)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants