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

fix: ensure bundled version of path-to-regexp is used #74

Merged
merged 2 commits into from
Mar 9, 2022

Conversation

rchl
Copy link
Collaborator

@rchl rchl commented Mar 8, 2022

We need to create an alias for path-to-regexp so that the client-side code resolves it to the version bundled with the module and not to whatever is hoisted to the root of node_modules. I'm using the same trick in nuxt-i18n for example, as suggested by Nuxt lead developers.

Also improved the error message on failing to parse path params to give more context to the error and help with finding the faulty route.

@codecov
Copy link

codecov bot commented Mar 8, 2022

Codecov Report

Merging #74 (4d8bfc5) into master (6f520cd) will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #74      +/-   ##
==========================================
+ Coverage   92.16%   92.22%   +0.06%     
==========================================
  Files           5        5              
  Lines         574      579       +5     
==========================================
+ Hits          529      534       +5     
  Misses         45       45              
Impacted Files Coverage Δ
lib/module.js 81.39% <100.00%> (+0.44%) ⬆️
lib/utility/controllers.js 94.40% <100.00%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f520cd...4d8bfc5. Read the comment docs.

@rchl
Copy link
Collaborator Author

rchl commented Mar 8, 2022

I've also pushed this change to beta branch and created a 5.0.0-beta.1 version.
Would appreciate if you could npm publish --tag beta it, assuming that no further changes are requested here.

@rchl rchl requested a review from ezypeeze March 8, 2022 20:37
@ezypeeze
Copy link
Owner

ezypeeze commented Mar 9, 2022

I've also pushed this change to beta branch and created a 5.0.0-beta.1 version. Would appreciate if you could npm publish --tag beta it, assuming that no further changes are requested here.

done

Do you mind to use this PR and also fix this annoying warnings?
image

@rchl
Copy link
Collaborator Author

rchl commented Mar 9, 2022

Thank you. I will fix those in a separate PR. Don't want to cloud those changes. :)

@rchl
Copy link
Collaborator Author

rchl commented Mar 9, 2022

I've tested those changes in my service and I'm happy with them so feel free to merge.

@rchl
Copy link
Collaborator Author

rchl commented Mar 9, 2022

Fixed here after all since those are just two small issues.

@ezypeeze ezypeeze merged commit 2ee628b into master Mar 9, 2022
@rchl rchl deleted the fix/params-match branch March 15, 2022 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants