-
-
Notifications
You must be signed in to change notification settings - Fork 159
deps: manual upgrade to express 5.1.0 #1626
Conversation
Merge the PR TriliumNext/express-partial-content#1. You can test this again I think. |
we will need a new released version on npm of that -> there is a CI action that will apparently do all of that, when there is a new tag created, however I don't want to "just do it" without @eliandoran having a final look over it :-) |
our code is already compliant with v5, and is not affected by the breaking changes described here: https://expressjs.com/en/guide/migrating-5.html I ran their codemod command, and it did not find anything – so I also double-checked by manually checking for all of the described changed topics in the migration guide. with this change npm will still print a warning, due to `@triliumnext/express-partial-content@"1.0.1"`, which needs to be updated to v5 as well
now we have full express 5 compatibility
FYI: I've updated the
and
the issue here is that the suggested way to migrate is not fully working with what I see we do on our end. Express v4:here we use and when I trigger that via a PUT, and do some loggin of the req data (name and value), I see (e.g. here I changed locale to en): params: {"0":"","name":"locale","value":"en"} If I force additional paths in the PUT, it will only use the first path part as "value", e.g. with: params: {"0":"/something/else","name":"locale","value":"de"} Express v5:here we are supposed to use params: {"name":"locale","value":["de"]} which results in an error, due to the value being in an array: Updating option 'locale' to 'de' same with me trying to add some fake paths as well: params: {"name":"locale","value":["de","something","else"]} Updating option 'locale' to 'de,something,else' I'll work this week on getting these fixed and potentially do some extra testing on those other paths as well, just to make sure |
I have a "naive" working solution, but need chck and consider some edgecases (e.g. values with special characters -> URI encode them or not? I'll need to compare what v4 was doing and mimick accordingly) some testing undergoing here still, should be back with an update this evening. |
…value*" the updateOption function that handles the req.param is just destructuring `const { name, value } = req.params;` and does nothing else with the path or any params. The remaining parts of the wildcard (which can be accessed via req.param[0]) are just ignored here. even with express v4, this would *always* just take and process the very first part of the path, in the exact wildcard's place, e.g. `/api/options/locale/de` and `/api/options/locale/de/test/whatever` would *both* end up destructuring "value" from req.param as "de" (because it is in the exact place of the 'value' wildcard) in express v5 the wildcard behaviour changes -> here req.param.value would return an array with the paths split into separate string. but since the code previously regarded only the first part of the path -> we can just get rid of the wildcard and use a named route param the only thing to keep in mind: if a request with more than one "value" is received, (e.g. `/api/options/locale/de/test/whatever`) -> since we don't have the wildcard anymore -> this will turn to a 404. IMHO that is actually desirable here though
|
|
marked PR as ready for review -> kindly also check my git commit messages for further info |
This PR will supersede the automatically created one: #1586
Our code is already compliant with v5, and is not affected by the breaking changes described here: https://expressjs.com/en/guide/migrating-5.html
I ran their codemod command, and it did not find anything (except it strangely reformatted some unrelated code…).
So I also double-checked manually for all of the described changed topics in the migration guide.
→ nothing there :-)
with this change npm will still print a warning, due to
@triliumnext/express-partial-content@"1.0.1"
, which needs to be updated to v5 as well, but the PR is raised TriliumNext/express-partial-content#1once that is in, I will need to update that dependecy as well and we can get this merged.
target is: after the next stable, as confirmed here:
#1586 (comment)