Skip to content

use console.log instead of context.log #36

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

Merged
merged 2 commits into from
Dec 6, 2022
Merged

Conversation

Skn0tt
Copy link
Contributor

@Skn0tt Skn0tt commented Dec 5, 2022

console.log has been equivalent to context.log for a while now, let's also show that in the examples.

@netlify
Copy link

netlify bot commented Dec 5, 2022

Deploy Preview for private-cdn-edge-functions-examples ready!

Name Link
🔨 Latest commit cd75912
🔍 Latest deploy log https://app.netlify.com/sites/private-cdn-edge-functions-examples/deploys/638f3f4c6f9e54000819e8ec
😎 Deploy Preview https://deploy-preview-36--private-cdn-edge-functions-examples.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Dec 5, 2022

Deploy Preview for candid-travesseiro-824c81 ready!

Name Link
🔨 Latest commit cd75912
🔍 Latest deploy log https://app.netlify.com/sites/candid-travesseiro-824c81/deploys/638f3f4cbb05800008137c6c
😎 Deploy Preview https://deploy-preview-36--candid-travesseiro-824c81.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify-staging-app
Copy link

netlify-staging-app bot commented Dec 5, 2022

Deploy Preview for edge-functions-examples ready!

Name Link
🔨 Latest commit cd75912
🔍 Latest deploy log https://app.netlifystg.com/sites/edge-functions-examples/deploys/638f3f4cc09bc70009393b95
😎 Deploy Preview https://deploy-preview-36--edge-functions-examples.netlifystg.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@Skn0tt Skn0tt requested a review from whitep4nth3r December 5, 2022 17:41
@netlify
Copy link

netlify bot commented Dec 5, 2022

Deploy Preview for edge-functions-examples ready!

Name Link
🔨 Latest commit cd75912
🔍 Latest deploy log https://app.netlify.com/sites/edge-functions-examples/deploys/638f3f4c5af643000844de31
😎 Deploy Preview https://deploy-preview-36--edge-functions-examples.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@whitep4nth3r
Copy link
Contributor

Thanks for this, @Skn0tt!

Do we want to add some explainer text saying that you can use both context.log and console.log? Do we want to say they are both the same? If this is a moot point, do we just want to update everywhere to use console.log?

@Skn0tt
Copy link
Contributor Author

Skn0tt commented Dec 6, 2022

From my understanding, there's no functional benefit in using context.log over console.log. For simplicity, I think it'd be good to only have one.
I'd remove all mentions of context.log, maybe even from the docs, and maybe even @deprecate it in the code and tell devs to use console.log instead? Though that might go a bit too far. Curious for @eduardoboucas's thoughts here.

@eduardoboucas
Copy link
Member

I agree with @Skn0tt.

@whitep4nth3r
Copy link
Contributor

I can go for that. Would you like to replace all mentions of context.log with console.log in this PR? There's also the question of whether we remove this example entirely, or just change it to console.log and remove the "Pro tip!" section.

@Skn0tt
Copy link
Contributor Author

Skn0tt commented Dec 6, 2022

I opened a tracking issue for deprecating context.log here: https://github.com/netlify/pod-compute/issues/316

@Skn0tt
Copy link
Contributor Author

Skn0tt commented Dec 6, 2022

I updated this PR so that it removes every mention of context.log in this codebase. I'm torn on removing the example entirely tbh - at the one hand it looks obvious that console.log is the one to use (if not documented otherwise), on the other hand, surely another example doesn't hurt (even if obvious)? I'll defer to you @whitep4nth3r if that's okay.

@whitep4nth3r
Copy link
Contributor

Yeah I'm good with this!

@whitep4nth3r
Copy link
Contributor

Why can't I see this deploy preview, though?

@Skn0tt
Copy link
Contributor Author

Skn0tt commented Dec 6, 2022

Not sure of the exact reason, but that contains private-cdn-.... - maybe it's a testing site?

@Skn0tt
Copy link
Contributor Author

Skn0tt commented Dec 6, 2022

@whitep4nth3r
Copy link
Contributor

Hmm that's weird — it's the deploy preview link provided by the PR! Anyway, looks good. Approved. And thank you!

@Skn0tt Skn0tt merged commit e4f3da1 into main Dec 6, 2022
@whitep4nth3r whitep4nth3r deleted the context-log-console-log branch December 6, 2022 13:43
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.

3 participants