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(live): make LIVE_URL env var available in --live mode #5607

Merged
merged 6 commits into from
Apr 12, 2023

Conversation

biilmann
Copy link
Member

@biilmann biilmann commented Apr 8, 2023

Summary

Before this fix there was no way to access the live URL from functions or build plugins when running netlify dev --live.

It might even be expected the URL or DEPLOY_URL would map to the live url, but I wanted to make sure I didn't introduce any unexpected behavior.

Oddly, there's a BASE_URL env var set by the handleLiveTunnel function that I'm not sure is documented anywhere and that doesn't seem very descriptively named.

Again, to avoid causing regressions for anything relying on this BASE_URL I added an additional LIVE_URL instead of changing is.

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

DALL·E 2023-04-07 18 55 02 - A picture of a cute animal (not mandatory, but encouraged)

Before this fix there was no way to access the live URL from functions
or build plugins.
@biilmann biilmann added type: bug code to address defects in shipped code low-risk labels Apr 8, 2023
@github-actions
Copy link

github-actions bot commented Apr 8, 2023

📊 Benchmark results

Comparing with 80b575e

Package size: 306 MB

⬇️ 0.00% decrease vs. 80b575e

^                                                                  302 MB  306 MB  306 MB  306 MB  306 MB 
│                                                                   ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐  
│                                                  271 MB  271 MB   |  |    |  |    |  |    |  |    |▒▒|  
│ ─264 MB──264 MB──264 MB──264 MB──264 MB──264 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.

@biilmann Can you share some more details about where you've tried to access the URL from? I just created a basic function and I see the URL variable pointing to the live URL:

const { env } = require("process");

module.exports.handler = async () => ({
  statusCode: 200,
  body: env.URL,
});
% curl https://eduardo-tests-de6a54.netlify.live/.netlify/functions/test-js
https://eduardo-tests-de6a54.netlify.live

It feels like this is the expected behaviour, and I'd like to get to the bottom of what's working and not working before introducing a new environment variable, because that has implications like figuring out what happens if someone is already using a variable with that name on their site, etc.

@biilmann
Copy link
Member Author

Hmm, it might be more of a niche bug than I thought, I was accessing the process.URL environment variable from a build plugin using the onPreDev hook, and there I never get the live url...

@eduardoboucas
Copy link
Member

Ah, that definitely sounds like a bug.

@eduardoboucas
Copy link
Member

I've opened an issue for this: #5612

@biilmann
Copy link
Member Author

Yeah, dug in more. Disregard this PR for now, the issue sticks a bit deeper, in terms of build plugins always getting the wrong URL and DEPLOY_URL. I'm investigating.

turns out that it's only builds and build plugins that have this problem
and that functions and edge functions already pick up the right URL and DEPLOY_URL
in the environment. This fixes the URL for builds and build plugins when --live is
set, but the problem still remains when not running in dev mode.
Now the dev server URL is always correctly set as URL and DEPLOY_URL for builds and
build plugins in dev mode
@biilmann
Copy link
Member Author

PR updated, this should be a real fix for the issue of URL and DEPLOY_URL behaving weird for builds and build plugins in dev mode.

So far the URl was always set to the main url for the site for build plugins and builds run as part of netlify dev and the DEPLOY_URL was always a broken URL (with 0 as deploy_id).

This moves up the URL definition to before any of the dev servers are started and passes the URL and DEPLOY_URL for the neetlify dev server as env overrides for the build command.

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! Left a question about a log line, but other than that this is good to go. 🚀

@@ -124,9 +125,20 @@ const dev = async (options, command) => {
startPollingForAPIAuthentication({ api, command, config, site, siteInfo })
}

const liveTunnelUrl = await handleLiveTunnel({ options, site, api, settings })
const url = liveTunnelUrl || getProxyUrl(settings)
process.env.URL = url
Copy link
Member

Choose a reason for hiding this comment

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

Note for our future selves: it would be nice if we could use this mechanism for setting internal environment variables rather than setting them on process.env directly, but that would mean moving the injectEnvVariables method further down and we'd need to look into any side-effects of that.

Not worth the hassle for now.

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.

I pushed df4ab5e with a couple of linting fixes. It's good to go now. 🚢

Thanks @biilmann! 🙌🏻

@eduardoboucas eduardoboucas enabled auto-merge (squash) April 12, 2023 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
low-risk type: bug code to address defects in shipped code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants