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

CDN support #92

Open
stereobooster opened this issue Jan 2, 2018 · 10 comments
Open

CDN support #92

stereobooster opened this issue Jan 2, 2018 · 10 comments

Comments

@stereobooster
Copy link
Owner

stereobooster commented Jan 2, 2018

Related: chrisvfritz/prerender-spa-plugin#114

config can look like:

"reactSnap": {
  "cdn": {
    "https://mysite.mycdn.com/assets": "build/assets"
  }
}

And code can be like

await page.setRequestInterception(true)
page.on('request', request => {
  if (request.url.startsWith(cdnUrl)) {
    // serve file from local file system instead of network
  }
})

Right now I’m wary of increasing surface area for bugs. I don’t want to make this as complicated as WDS client. I still plan to make changes to it, so I intend to keep it scoped to features CRA supports, and serving JS from a different host or port is not supported right now.

CRA on CDN_URL

@kirkchris
Copy link

Would love to have this. Right now we're building the site and uploading new assets to CDN and then just loading them remotely during our snapshotting. However this introduces some other issues because I'd like to use skipThirdPartyRequests and am unable to do so because our CDN url is a third party. Not skipping third party requests right now is causing some weirdness with stripe JS.

Another potential enhancement that is somewhat different, but slightly related, would be to enable skipThirdPartyRequests to support a new option of skipThirdPartyRequestsExcept as an array (or flipped baseName as an array) that way we can conditionally allow some third party requests to be loaded as needed. Happy to create a separate issue tagged under enhancement for this.

If you have any ideas on how to do this with the current version, would love to hear.

Great work on the package!

@stereobooster
Copy link
Owner Author

skipThirdPartyRequestsExcept doesn't sound good to me

@kirkchris
Copy link

I'm very open on naming if that's what you meant...

@stereobooster
Copy link
Owner Author

I mean it does not feel right. If something like this starts to appear it means that abstraction is wrong - either tool applied to wrong task or tool is wrong. If the original idea will fix your issue we should do that

@kirkchris
Copy link

Yeah, I understand what you mean. It gets us close, however we also have a section of our site on the public facing area (that we are snapshotting) that loads content in from the Contentful CMS. So there is a case in which I want to allow some third party requests but not others. Potentially for my use case, I need to just allow third party requests and strip out the stripe.js loader during snapshotting, then add it back in to HTML afterwards.

@stereobooster
Copy link
Owner Author

Thinking out loud.

There is also proxy from create-react-app (#101), which translates path to url and works on the server.

Proposed cdn config translates url to path and works in the browser.

It can also translate from url to url - in our case, it will translate from given url to exactly the same url and can be expressed as:

"reactSnap": {
  "cdn": {
    "https://mysite.mycdn.com/assets": "build/assets", // url -> paht
    "https://api.contentfull.com": true // url -> url
  }

But the name cdn doesn't fit in this case. Also option cdn will always take precedence over skipThirdPartyRequests and will work independently of skipThirdPartyRequests config.

@kirkchris
Copy link

That could definitely work. Agree that cdn should work independently of skipThirdPartyRequests as it's separate and actually by nature, bypassing something even being a third party request.

I think the name proxy could even be a better name than cdn in config, as that's really what this does and also opens up for more flexibility, as you showed above. Plus allows for support if they had multiple CDN urls for different types of content or something.

@kirkchris
Copy link

Created a PR (#136) with the changes discussed. Happy to continue the discussion and make changes.

@stereobooster
Copy link
Owner Author

It can happen that I over complicated the task. Maybe publicUrl would be enough, which would correspond to Create React App %PUBLIC_URL%. What is the case for multiple urls?

Related:

@baptooo
Copy link

baptooo commented Jul 18, 2018

Hello @stereobooster thank you for the library it really helps !

For this subject I just want to share with you a (bit dirty...) hack I found to be able to prevent some scripts from loading while ReactSnap is crawling

<boby>

  <div id="third-party-container"></div>
  <script>
    if (navigator.userAgent === 'ReactSnap')  {
      document.write('<textarea id="third-party">');
    }
  </script>
  <script src="https://example.com/third-party-script.js"></script>
  </textarea>

  <script>
    window.snapSaveState = () => {
      document.getElementById('third-party-container').innerHTML =
        document.getElementById('third-party').value;
    }
  </script>
</body>

To explain a bit:

The main idea is to condition the scripts with a <textarea /> element that will be enabled only
when ReactSnap is crawling.

So for the following cases:

Local dev server
The <textarea /> won't be written so the scripts will load

Crawling with react-snap
The <textarea /> will be written until the page is fully crawled and then the snapSaveState callback will move the scripts to third-party-container and remove the <textarea /> to prevent from polluting the HTML

Snapshot built
The scripts will be inside third-party-container.

Caveat

The main issue is that your app built without react-snap will have a closing </textarea> tag that will invalidate your HTML...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants