Skip to content
This repository has been archived by the owner on Jul 12, 2019. It is now read-only.

fix regression where env is ignored #154

Merged
merged 2 commits into from
May 29, 2019
Merged

fix regression where env is ignored #154

merged 2 commits into from
May 29, 2019

Conversation

xavdid
Copy link
Contributor

@xavdid xavdid commented May 29, 2019

Ok! So this fixes https://github.com/zapier/zapier/issues/26273. The root issue was that the early require(appPath). Imagine you have the following:

// app/index.js
module.exports = {
  // ... whatever
  url: `${process.env.BASE_URL}/oauth`,
}

when you require('./app'), process.env.BASE_URL is evaluated. The only problem is, the env being passed into lambda from the server isn't merged into process.env until this line, so app.url is undefined/oauth. When it's required again as part of loadApp further down, the result is cached. So even though process.env.BASE_URL exists, the url is wrong.

I read carefully through everything the http patch does and moving it further into that method should have no ill effects (since nothing between its original and new position can use the http module). All the tests are passing as well. I also tested locally and the current env pops up as expected.

I tried the following in loadApp, but weirdly, it didn't work. I had sunk enough time into this as is, so I didn't dig much further after confirming the fix itself.

delete require.cache[require.resolve(appRawOrPath)]; // always get a fresh copy of the app
return resolve(require(appRawOrPath));

Copy link
Contributor

@codebycaleb codebycaleb left a comment

Choose a reason for hiding this comment

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

I love when really complex problems (env is being ignored in a very specific set of circumstances) have simple solutions (move the skipHttpPatch handling). 😁 Thanks for nailing this one down!

src/tools/create-lambda-handler.js Show resolved Hide resolved
Copy link
Contributor

@stevelikesmusic stevelikesmusic left a comment

Choose a reason for hiding this comment

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

Nice find @xavdid! All looks good 👍

@xavdid xavdid merged commit 48ddbd4 into master May 29, 2019
@xavdid xavdid deleted the fix-missing-env branch May 29, 2019 19:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants