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: function:serve shows undefined port in url #4146

Merged
merged 10 commits into from
Feb 4, 2022

Conversation

karagulamos
Copy link
Contributor

@karagulamos karagulamos commented Jan 30, 2022

Summary

Fixes #4116

image

When a Netlify site is served using netlify functions:serve locally, the function URL shown at the terminal contains an undefined port which is incorrect albeit confusing to users when they click on it. This is caused by the use of an undefined NetlifyFunction:settings:port property composed within the NetlifyFunction:url (getter) property. This PR fixes the issue and includes relevant tests.

NOTE: Although I scoured the codebase for a valid usage of NetlifyFunction:settings:port to no avail, I’m not particularly fond of the current solution but left it there with comments (and for compatibility) should anyone wish to take a closer look.

image

@github-actions github-actions bot added the type: bug code to address defects in shipped code label Jan 30, 2022
@github-actions
Copy link

github-actions bot commented Jan 30, 2022

📊 Benchmark results

Comparing with 474118d

Package size: 362 MB

(no change)

^  360 MB  360 MB  360 MB  360 MB  360 MB  360 MB  360 MB  360 MB  360 MB  360 MB  360 MB  360 MB  362 MB 
│   ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
└───┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴──>
    T-12    T-11    T-10    T-9     T-8     T-7     T-6     T-5     T-4     T-3     T-2     T-1      T    
Legend

@karagulamos karagulamos added the automerge Add to Kodiak auto merge queue label Jan 31, 2022
lukasholzer
lukasholzer previously approved these changes Jan 31, 2022
src/lib/functions/netlify-function.js Outdated Show resolved Hide resolved
@lukasholzer
Copy link
Contributor

Thx for adapting the tests! 🐳

@lukasholzer lukasholzer removed the automerge Add to Kodiak auto merge queue label Jan 31, 2022
@karagulamos karagulamos added automerge Add to Kodiak auto merge queue and removed automerge Add to Kodiak auto merge queue labels Jan 31, 2022
@erezrokah
Copy link
Contributor

@karagulamos, do you mind resolving the merge conflict? The test file was renamed in #4164

@karagulamos karagulamos self-assigned this Feb 4, 2022
@erezrokah erezrokah requested a review from lukasholzer February 4, 2022 12:41
Copy link
Contributor

@erezrokah erezrokah left a comment

Choose a reason for hiding this comment

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

Going to approve per @lukasholzer review so we can merge this

@erezrokah erezrokah added the automerge Add to Kodiak auto merge queue label Feb 4, 2022
@kodiakhq kodiakhq bot merged commit db0f1ed into main Feb 4, 2022
@kodiakhq kodiakhq bot deleted the fix/function-serve-url-port-undefined branch February 4, 2022 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Add to Kodiak auto merge queue type: bug code to address defects in shipped code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

netlify functions:serve shows an undefined port in loaded function URL
3 participants