Skip to content

Remove unused originalIndex from acceptParams#5119

Merged
dougwilson merged 1 commit intoexpressjs:masterfrom
raksbisht:fixes/remove-unused-originalindex-from-acceptParams
Feb 22, 2023
Merged

Remove unused originalIndex from acceptParams#5119
dougwilson merged 1 commit intoexpressjs:masterfrom
raksbisht:fixes/remove-unused-originalindex-from-acceptParams

Conversation

@raksbisht
Copy link
Copy Markdown
Contributor

@raksbisht raksbisht commented Feb 8, 2023

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

@dougwilson
Copy link
Copy Markdown
Contributor

dougwilson commented Feb 8, 2023

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
Copy Markdown
Contributor

dougwilson commented Feb 8, 2023

Apologies, acceptParams is not an exported function I think.

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

raksbisht commented Feb 8, 2023

Thanks @dougwilson!

LinusU
LinusU reviewed Feb 21, 2023
View reviewed changes
LinusU
LinusU approved these changes Feb 21, 2023
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor

dougwilson commented Feb 21, 2023

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
Copy Markdown
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
@raksbisht @neohotsauce
lint: remove unused parameter from internal function
0876eaa 
coses expressjs#5119
@UlisesGascon UlisesGascon mentioned this pull request Feb 26, 2024
Merged
@davidshen84 davidshen84 mentioned this pull request Mar 25, 2024
Merged
This was referenced Mar 26, 2024
Open
Open
Open
Open
Open
Open
This was referenced Mar 28, 2024
Merged
Merged
@ajfisher ajfisher mentioned this pull request Mar 29, 2024
Merged
@liaocanada liaocanada mentioned this pull request Mar 29, 2024
Merged
@somabc somabc mentioned this pull request Mar 29, 2024
Merged
github-actions bot pushed a commit to brafdlog/caspion that referenced this pull request Apr 2, 2024
@semantic-release-bot
chore(release): 1.26.0 [skip ci]
ada789f 
# [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)
@mnathsnyk mnathsnyk mentioned this pull request May 16, 2024
Open
@X-oss-byte X-oss-byte mentioned this pull request May 18, 2024
Open
@pradumchintamani pradumchintamani mentioned this pull request Oct 4, 2024
Open
@niabhail niabhail mentioned this pull request Oct 4, 2024
Open
@MaximKing1 MaximKing1 mentioned this pull request Oct 4, 2024
Open
@regreaves regreaves mentioned this pull request Oct 4, 2024
Open
@Dinesh-Bhogadi Dinesh-Bhogadi mentioned this pull request Oct 4, 2024
Open
@X-oss-byte X-oss-byte mentioned this pull request Oct 4, 2024
Open
@Nibeth Nibeth mentioned this pull request Oct 4, 2024
Open
@NOUIY NOUIY mentioned this pull request Oct 5, 2024
Closed
@scatools-demo scatools-demo mentioned this pull request Oct 5, 2024
Open
@ranjith221132 ranjith221132 mentioned this pull request Oct 5, 2024
Open
@ramachoon ramachoon mentioned this pull request Oct 5, 2024
Open
@PugChungus PugChungus mentioned this pull request Oct 5, 2024
Open
@TheLastArchive TheLastArchive mentioned this pull request Oct 5, 2024
Open
@Erenkrs Erenkrs mentioned this pull request Oct 5, 2024
Open
@IzzyDotExe IzzyDotExe mentioned this pull request Oct 5, 2024
Open
@RichardSherrard RichardSherrard mentioned this pull request Oct 5, 2024
Open
@pradumchintamani pradumchintamani mentioned this pull request Oct 5, 2024
Open
@regreaves regreaves mentioned this pull request Oct 5, 2024
Open
@MaximKing1 MaximKing1 mentioned this pull request Oct 5, 2024
Open
@dronixpro dronixpro mentioned this pull request Oct 5, 2024
Open
@Dinesh-Bhogadi Dinesh-Bhogadi mentioned this pull request Oct 5, 2024
Open
@X-oss-byte X-oss-byte mentioned this pull request Oct 5, 2024
Open
@jbalestrSnyk jbalestrSnyk mentioned this pull request Oct 6, 2024
Open
@PugChungus PugChungus mentioned this pull request Oct 6, 2024
Open
@Danielhay016 Danielhay016 mentioned this pull request Oct 6, 2024
Open
@Erenkrs Erenkrs mentioned this pull request Oct 6, 2024
Open
@Balaji-Balaraman Balaji-Balaraman mentioned this pull request Oct 6, 2024
Open
@Dinesh-Bhogadi Dinesh-Bhogadi mentioned this pull request Oct 6, 2024
Open
@pradumchintamani pradumchintamani mentioned this pull request Oct 6, 2024
Open
@regreaves regreaves mentioned this pull request Oct 6, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@LinusU LinusU LinusU approved these changes

Assignees

No one assigned

Labels

pr

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@raksbisht @dougwilson @LinusU