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

feat: print function URL on first load (closes #3618) #3646

Merged
merged 13 commits into from Dec 21, 2021
Merged

feat: print function URL on first load (closes #3618) #3646

merged 13 commits into from Dec 21, 2021

Conversation

netlify-team-account-1
Copy link
Contributor

Summary

Fixes #3618


For us to review and ship your PR efficiently, please perform the following steps:

  • Open a bug/issue before writing your code πŸ§‘β€πŸ’». This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire πŸ”₯ (e.g. incident related), you can skip this step.
  • Read the contribution guidelines πŸ“–. This ensures your code follows our style guide and
    passes our tests.
  • Update or add tests (if any source code was changed or added) πŸ§ͺ
  • Update or add documentation (if features were changed or added) πŸ“
  • Make sure the status checks below are successful βœ…

A picture of a cute animal (not mandatory, but encouraged)

@netlify-team-account-1 netlify-team-account-1 requested review from eduardoboucas and removed request for eduardoboucas November 15, 2021 16:15
@github-actions github-actions bot added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Nov 15, 2021
@github-actions
Copy link

github-actions bot commented Nov 15, 2021

πŸ“Š Benchmark results

Comparing with 17dc0b6

Package size: 357 MB

⬆️ 0.52% increase vs. 17dc0b6

^  358 MB  358 MB  358 MB  358 MB  358 MB  358 MB  358 MB  358 MB  354 MB  354 MB  354 MB  354 MB  357 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

Copy link
Member

@eduardoboucas eduardoboucas left a comment

Choose a reason for hiding this comment

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

LGTM!

My only concern is that this adds a lot of noise to the output, especially when there are dozens of functions. Rather than adding the full URL to the log line, could we experiment with using something like https://www.npmjs.com/package/terminal-link?

@netlify-team-account-1 would you mind adding a screenshot of what the output looks like with some functions, and then we ask @joeyaiello for his thoughts?

@netlify-team-account-1
Copy link
Contributor Author

Sure thing! @joeyaiello what are your thoughts on this?
Screenshot 2021-11-18 at 07 56 49

@netlify-team-account-1
Copy link
Contributor Author

@joeyaiello @eduardoboucas WDYT about moving forward with this and then potentially incrementing on smth like terminal-link?

minivan
minivan previously approved these changes Dec 6, 2021
@netlify-team-account-1
Copy link
Contributor Author

Added terminal-link, this is how it looks on iTerm2:

Screenshot 2021-12-06 at 18 12 34

Unfortunately, it works neither in standard MacOS terminal, nor in the VSCode terminal, nor in Hyper (even thought that's on the list of supported terminals!). When it doesn't work, the fallback looks exactly as the original screenshot,, so it's still kind of cluttered.

An alternative would be to keep the current behaviour of not printing the URL, and only terminal-link for supported terminals where it doesn't add any visual clutter.

@eduardoboucas
Copy link
Member

Can we change the fallback to be just the URL in parenthesis, instead of showing "available under"?

-Loaded function echo available under: http://localhost:8080/.netlify/functions/echo
+Loaded function echo (http://localhost:8080/.netlify/functions/echo)

@netlify-team-account-1
Copy link
Contributor Author

yes! implemented in e722ad1

eduardoboucas
eduardoboucas previously approved these changes Dec 6, 2021
Copy link
Member

@eduardoboucas eduardoboucas left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@eduardoboucas eduardoboucas left a comment

Choose a reason for hiding this comment

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

πŸš€

@lukasholzer lukasholzer added the automerge Add to Kodiak auto merge queue label Dec 20, 2021
@kodiakhq kodiakhq bot removed the automerge Add to Kodiak auto merge queue label Dec 20, 2021
@kodiakhq
Copy link
Contributor

kodiakhq bot commented Dec 20, 2021

This PR currently has a merge conflict. Please resolve this and then re-add the automerge label.

@netlify-team-account-1 netlify-team-account-1 added the automerge Add to Kodiak auto merge queue label Dec 21, 2021
@kodiakhq kodiakhq bot merged commit d6aa3fa into main Dec 21, 2021
@kodiakhq kodiakhq bot deleted the fix-3618 branch December 21, 2021 08:41
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: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

As a new user I don't know how to invoke my functions when served with ntl dev
5 participants