Fix authentication on Glitch #1303
Conversation
lfdebrux
force-pushed
the
ldeb-fix-glitch
branch
from
1a86099
to
a58b013
Compare
lfdebrux
force-pushed
the
ldeb-fix-glitch
branch
2 times, most recently
from
84f8089
to
4a91593
Compare
We no longer need this code, as express does the parsing of x-forwarded-proto for us [[1]]. [1]: https://github.com/expressjs/express/blob/4.17.1/lib/request.js#L316-L323
|
The tests are currently passing, but Jest is complaining about open handles, due to 'asynchronous operations that weren't stopped in your tests'. I've isolated the issue down to the changes in |
|
tested and got this working on Glitch, however Update Both files are only created by |
|
This is a good fix to have, but in terms of suggesting it as a fix for the heroku situation, I'm not sure we know what the new workflow is. In addition to the |
Ah, I see what happened, I assumed that |
Good questions, but let's discuss on Slack, or an a different issue, as it isn't really relevant for this PR! |
I think this is expected, the ``
It might also be that Glitch treats |
Refactor code to use `inProduction` function instead of checking environment variables directly. Hopefully this should a) reduce duplication b) make programmer intent clearer, thus making maintenance easier. This has the side effect of fixing issue #1302, because now we don't run BrowserStack if serving from Glitch.
Browsersync doesn't play well with Glitch, so now it won't run if your prototype is on Glitch, even if you have `useBrowserSync: true` in your config file. I think this shouldn't actually affect users, because using Browsersync with Glitch doesn't make much sense (it won't work, and Glitch auto-relaods for you anyway), but this commit adds a comment anyway to make it clear that Browsersync is only used for development environments (the config variable has always been ignored when NODE_ENV is true).
|
@lfdebrux just to note, my experience when I tried this was to import the prototype from GitHub. The blank project has no If this experience is the same for others, at the least we'd need to tell people to make a |
Yeah, we have something explaining this in our draft Glitch docs, similar to what we have in our Heroku docs. See ticket prototype-team-internal#14, or prototype-team-internal#16 for where we are currently with Glitch. |
|
ah thanks, hadn't seen that! |
Fixes #1302.
Browsersync and Glitch don't work well together. Currently when on Glitch, Browsersync runs, and for reasons I don't understand this stops the authentication working (you can never log in).
As a fix/workaround, this PR changes
listen-on-port.jsto not use Browsersync if the prototype is running on Glitch. I think this shouldn't be a problem for users, because using Browsersync with Glitch doesn't make much sense (it won't work, and Glitch auto-relaods for you anyway).With this PR I've tried to follow the maxim of 'leave the code better than you found it', so the way I've approached the fix is to take the logic of determining whether we're running on Glitch, and whether we're running on production, and make a function in utils, with tests, that can be used in all the places we were previously checking environment variables in various different ways.
I also removed some Glitch related code that wasn't needed, as ExpressJS does the same thing for us, and added a comment to
config.jsmaking it clear that Browsersync won't run if your prototype is published (whether with Heroku or Glitch).