-
Notifications
You must be signed in to change notification settings - Fork 392
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
Proxy / CDN Config Implementation #136
base: master
Are you sure you want to change the base?
Conversation
@@ -29,6 +29,7 @@ const defaultOptions = { | |||
puppeteerExecutablePath: undefined, | |||
puppeteerIgnoreHTTPSErrors: false, | |||
publicPath: "/", | |||
proxy: {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is proxy config in CRA which I plan eventually support, so there is a name clash. Not sure what to do about it. Otherwise PR looks fine, but haven't tested it myself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified in our own deploy and code works. We're using it in production now.
Any thoughts on name? I can use cdn
if you think that's better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Three options (which I do not like)
- proxy (good, but clashes with other one)
- browserProxy
- cdn
- thirdPartyResources
There are only two hard problems in the programming...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bah, sorry about the delay on this... I didn't have any brilliant ideas on the naming 😒 I think of those, I probably like thirdPartyResources
the best. My concern with cdn
is that it wouldn't always be a cdn, just any third party. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are not delaying it. It is rather me, who have not much time recently to do updates on this project. I didn't expect that you alone should come up with the name. I still have no good idea either
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pardon my stupidity, but how does this clash? i use CRA and reactSnap's config is a separate object, so reactSnap.proxy
doesn't conflict with proxy
with CRA uses. though if it does, what do you think of snapProxy
for a name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after trying to write tests for this in #179, this seems very different to CRA's proxy. this would be better called cdn
to avoid confusion.
At the moment I'm trying to write proper tests for the package. It was without tests way too long. This means, that all other PRs will be on hold. See #171 |
ae782b6
to
193b27b
Compare
I am currently testing out react-snap with my current project where I had setup an api proxy, as per the CRA documentation, to handle API calls to the backend. When the react-snap script runs after building the project, I checked the response for one of my API calls and I was getting the html markup of my index.html file instead of the expected JSON body. Just wanted to know if I've missed something as I have looked around and could not find anything to solve my issue. I've tried replacing one of my fetch API urls with the absolute path of the API end point and I am now getting the correct response. But I don't want to do it this way for security reasons. |
@stereobooster any chance this might get added?
React-snap is really great, but this feature is missing. I'm aware it wasn't requested a lot, but the issue related to this is from 2 Jan 2018. |
any news on this? i haven't found any way to get a proxy working during the site build and it's a big pain point |
Regarding #92
Happy to make changes based on feedback.