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

Page data is trying to be loaded from the path prefix #6

Closed
blessanm86 opened this issue Jul 11, 2019 · 43 comments · Fixed by #7 or gatsbyjs/gatsby#16122
Closed

Page data is trying to be loaded from the path prefix #6

blessanm86 opened this issue Jul 11, 2019 · 43 comments · Fixed by #7 or gatsbyjs/gatsby#16122

Comments

@blessanm86
Copy link

blessanm86 commented Jul 11, 2019

Hi, I just tried this plugin as my page was redirecting after it loaded.

So basically I have gatsby site running from example.s3.aws.com with path prefix gatsby
There is one route for this site example.s3.aws.com/de-DE/summer-dresses

So I configured my proxy (In house proxy named skipper used in my company)

PathSubtree("/clp/:clpSlug")
  -> setPath("/de-DE/${clpSlug}/index.html")
  -> "example.s3.aws.com";

Path("/gatsby/*asset")
  -> setPath("/${asset}")
  -> "example.s3.aws.com";

So now when I try to access the page from mydomain.com/clp/summer-dresses the page loads except one file which the page-data.json
It is looking for the file in the path mydomain.com/gastby/page-data/clp/summer-dresses/page-data.json and fails which makes sense.

Im wondering if this issue is related this issue.
Any help with this would be appreciated

@wardpeet
Copy link
Owner

wardpeet commented Jul 12, 2019

thanks for reporting! Skipper sounds a lot like Zalando 😂

It seems to be the correct behaviour. Pathprefix tels gatsby to look for everything in that folder. I'm not sure if you need pathprefix for your use case.

Where should gatsby look for the page-data.json in your use case? (mydomain.com/page-data/clp/summer-dresses/page-data.json)

@blessanm86
Copy link
Author

Yes I work for Zalando. I replaced the pathPrefix and instead use assetPrefix.

The page loads fine if I dont use this plugin but then if I access 'mydomain.com/clp/summer-dresses' the page loads but then url is now mydomain.com/de-DE/summer-dresses. I found that a navigate call is happening in production-app.js.

So I tried this plugin but now when I load 'mydomain.com/clp/summer-dresses' I see 3 page-data.json` being requested. Only 1 of them fails

mydomain.com/gatsby/page-data/de-DE/summer-dress-f/page-data.json (works)
mydomain.com/gatsby/page-data/clp/summer-dress-f/page-data.json (Fails)
mydomain.com/gatsby/page-data/404.html/page-data.json (works)

The page loads partially but with exceptions

loader.js:29 GET mydomain.com/gatsby/page-data/clp/summer-dress-f/page-data.json 404 (Not Found)
TypeError: Cannot read property 'json' of undefined
    at Object.children (production-app.js:55)

So I assumed I dont need the failing page-data.json. The first requested json has everything. This one is extra and not needed.

@blessanm86
Copy link
Author

blessanm86 commented Jul 12, 2019

From what I see this line from production-app.js is the problem.

loader.loadPage(browserLoc.pathname).then(page => {

Its trying to load the page-data from the browser path while the data is in the __BASE_PATH__ + pagePath.

I just noticed mydomain.com/gatsby/page-data/de-DE/summer-dress-f/page-data.json is not accessed via ajax but is included the html file as <link as="fetch" rel="preload" href="/zalonful/page-data/de-DE/summer-dress-f/page-data.json" crossorigin="use-credentials">

Im not completely sure what this page-data is needed for. This is how much I know so far.

@wardpeet
Copy link
Owner

sweet thanks, I forgot to override the preloader. Page-data is the manifest file per page to know which assets to load/preload for a given page.

@wardpeet
Copy link
Owner

I'll try to get it fixed today, if not I'll do it tomorrow.

@blessanm86
Copy link
Author

@wardpeet cool cool. thank you very much for looking into this.

@blessanm86
Copy link
Author

Hi Ward, is the fix ready yet?

@wardpeet
Copy link
Owner

wardpeet commented Jul 15, 2019

Published in version 0.1.1. Sorry for keeping you waiting.

@blessanm86
Copy link
Author

So I don't completely understand the plugin life cycle. But it seems the line in production app still gets executed and thus the error still happens.

Is the order of the plugin in gatsby-config relevant? Currently its the last plugin in the array.

@blessanm86
Copy link
Author

I assume the plugin is trying to override the loadPage from the loader.js. But the plugins loadPage is not getting invoked.

@wardpeet
Copy link
Owner

we still need to call loadPage for the current page to execute javascript. Any chance you can make a videor or share a public url with the issue you're having.

@blessanm86
Copy link
Author

Unfortunately we dont have a instance of our proxy(skipper) publicly available for development purposes.
Would it be ok for u to install it locally. Its pretty simple if you have GO on ur computer.

You just use the following commands

clone the repo and inside the repo
mkdir ws
cd ws
export GOPATH=$(pwd)
export PATH=$PATH:$GOPATH/bin
GO111MODULE=on go get github.com/zalando/skipper/...

Create example.eskip with the content

zalonful: PathSubtree("/clp/:clpSlug")
  -> setPath("/de-DE/${clpSlug}/index.html")
  -> "https://kitt-zalonful-test.s3.eu-central-1.amazonaws.com";

zalonfulAssets: Path("/zalonful/*asset")
  -> setPath("${asset}")
  -> "https://kitt-zalonful-test.s3.eu-central-1.amazonaws.com";

You can then run the proxy with skipper -routes-file example.eskip and access the page with the url http://localhost:9090/clp/summer-dress-f

The page is actually available at https://kitt-zalonful-test.s3.eu-central-1.amazonaws.com/de-DE/summer-dress-f/index.html but it wont fully work cause of the assetPath.

@blessanm86
Copy link
Author

@wardpeet I got a url that reproduces the issue now. It is https://www-de.release.zalando.net/zlnclp/summer-dress-f

@xavivars
Copy link

xavivars commented Jul 16, 2019

@Blessenm we're actually using skipper as well as a router (thanks for open-sourcing it, btw!).

I may be able to help you :)

What we're doing is we're not using pathprefix, but assetPrefix, so all static files are loaded from a different domain/path (in you case, it could either be https://kitt-zalonful-test.s3.eu-central-1.amazonaws.com/ or even https://www-de.release.zalando.net/zlnclp/zalonful/).

What is the path prefix that you're adding in your gatsby-config? Skipper instance seems to be down right now.

@wardpeet
Copy link
Owner

wardpeet commented Jul 17, 2019

Sorry for being late to the party. Is it possible to create a docker container which I can easily run?

Also could you share your gatsby-config with us?

We will have to download a page-data.json file. Where should they be located for your use case?

@wardpeet wardpeet reopened this Jul 17, 2019
@xavivars
Copy link

xavivars commented Jul 17, 2019

@wardpeet: does this works for you?

docker pull xavivars/skipper-gatsby-test

docker run -i -p 9090:9090 xavivars/skipper-gatsby-test

@blessanm86
Copy link
Author

@xavivars thanks for reproducing this. The docker image works.
I just realized the url i shared is only accessible within our internal network

@wardpeet The correct pagedata.json is already available at http://localhost:9090/zalonful/page-data/de-DE/summer-dress-f/page-data.json and included in the page with the link tag.

Here is my gatsby-config.js file

if (process.env.NODE_ENV !== "production") {
  require("dotenv").config({
    path: `.env`
  });
}

module.exports = {
  assetPrefix: "zalonful",
  siteMetadata: {
    title: "Gatsby Default Starter",
    description:
      "Kick off your next, great Gatsby project with this default starter. This barebones starter ships with the main Gatsby configuration files you might need.",
    author: "@gatsbyjs"
  },
  plugins: [
    {
      resolve: "gatsby-source-transifex",
      options: {
        apiKey: process.env.TRANSIFEX_API_KEY,
        organisationSlug: "zalando",
        projectSlug: "zalonful",
        resourceSlug: "messagesjson"
      }
    },
    {
      resolve: "gatsby-source-contentful",
      options: {
        spaceId: process.env.SPACE_ID,
        accessToken: process.env.ACCESS_TOKEN,
        host: process.env.CONTENTFUL_HOST,
        forceFullSync: true
      }
    },
    "gatsby-plugin-react-helmet",
    "gatsby-plugin-emotion",
    "gatsby-transformer-sharp",
    "gatsby-plugin-sharp",
    "@wardpeet/gatsby-plugin-static-site"
  ]
};

@xavivars
Copy link

xavivars commented Jul 17, 2019

I managed to find out what is happening, but I have no idea why:

image

@wardpeet any idea why that may be happening?

@xavivars
Copy link

Not sure if this is related, but I can't set up a breakpoint in Chrome console in loader.loadPage, while is perfectly possible to set it in loader.loadPageSync or loader.hovering

image

@brianbento
Copy link

@xavivars Possibly stupid question, is the loader not replaced because it's set as a const so it can't be changed?

https://github.com/gatsbyjs/gatsby/blob/a1ab8bc1d5befab1fdc9287de65837216439bb0d/packages/gatsby/cache-dir/production-app.js#L22

@xavivars
Copy link

I don't think so... we're actually not changing the loader itself, but some of its functions... and what I don't understand is why loader.loadPageSync and loader.hovering get properly replaced, but loader.loadPage isn't...

@wardpeet
Copy link
Owner

Thanks for all this info! I have found the issue, we override the window.__loader variable but we don't change any code that is used with import statements. I'll need to set the loader instance for gatsby and not just for the window object.

I have to think about this how I should solve this 🤔. xavivars any chance you can send me the source code of your docker stuff so I can fiddle with it 👍. This is really great! <3

@xavivars
Copy link

I mean, the docker stuff I built was just with @Blessenm's information, so not even sure it's worth publish 😝

But in any case, here you have! Glad to help (and selfish! I have identically the same problem!!! )

https://github.com/xavivars/skipper-gatsby-test

@xavivars
Copy link

xavivars commented Jul 19, 2019

Thanks for all this info! I have found the issue, we override the window.__loader variable but we don't change any code that is used with import statements. I'll need to set the loader instance for gatsby and not just for the window object.

But isn't this just holding a reference to the same instance? https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby/cache-dir/production-app.js#L28

@xavivars
Copy link

Tried to find out why this may be happening, but didn't succeed.

I still don't get why the instance of loader is not the same when accessing the it through window.__loader than when accessing it directly in production-app.js.

I may be missing something about how javascript handles global variables

@blessanm86
Copy link
Author

So I think the overwrite of the loader is happening from here in cache-dir/navigation.js.

So the default publicLoader gets set from loader.js

@xavivars
Copy link

But, as far as I read it in here, in production-app.js we're setting back the instance that publicLoader from loader.js will use... So I'm still missing something.

@blessanm86
Copy link
Author

@wardpeet Will you be able to look into this any time soon?

@xavivars
Copy link

@wardpeet @Blessenm I added a PR in gatsby core to fix the issue

@wardpeet
Copy link
Owner

wardpeet commented Jul 26, 2019

Sorry taking a few days holiday because it's so hot. I'll see if I can get to it in the weekend.

Link to PR?

@xavivars
Copy link

gatsbyjs/gatsby#16122

@blessanm86
Copy link
Author

So I played around with @xavivars changes in gatsby and tried to do a rebuild on the default starter blog with skipper infront of it.

This line fails as findMatchPath is not defined in the loader. It seems the default loader from loader.js is getting set and it doesn't have findMatchPath. I added that from the base loader and now it runs without exceptions.

But still the navigate function gets called from production-app.js and redirect happens. It seems like the navigate from the this plugin is not getting called.

@xavivars
Copy link

xavivars commented Aug 5, 2019

findMatchPath shouldn't be a blocker anymore, the PR just got merged.

@wardpeet
Copy link
Owner

wardpeet commented Aug 5, 2019

One of the last gatsby versions have broke this Plugin as well so I'm looking into this to get this fixed. I fixed the prodloader so I should have a PR up today or tomorrow

@blessanm86
Copy link
Author

@wardpeet sounds great. thanks

@blessanm86
Copy link
Author

@wardpeet im not sure, should the fix be in gatsby core or this plugin?

@xavivars
Copy link

xavivars commented Aug 8, 2019

Also interested on the answer for @Blessenm question

@xavivars
Copy link

xavivars commented Aug 12, 2019

@wardpeet any chances this gets done anytime soon (just to know it, not requesting it ;) )?

@blessanm86
Copy link
Author

@xavivars I just updated gatsby to the latest version. But when I enable this plugin the page fails with an exception from the cache/root.js

Are things working for u? and is this issue fixed?

@xavivars
Copy link

Last tests I did, all assets were being used downloaded properly, but react crashed (blank page) when rehydrating...

So in terms of:is fully working? No. Are all files being downloaded properly? Yes.

Still need to figure out what is happening with the rehydration, but I'm leaving for a long vacationing now, so I most likely I won't be able to dive into it any time soon

@apaniel
Copy link
Contributor

apaniel commented Sep 5, 2019

I tracked down the issue, only can reproduce it from gatsby 2.13.62 and below. in 2.13.63 manifest.webmanifest is forced to have the assetPrefix removed, which makes it crash when using this plugin, but that is to be fixed later, once we fix the current one.

It seems that loadPageSync is returning undefined, so in EnsureResources, specifically this line: pageResources: pageResources || loader.loadPageSync(location.pathname), pageResources are undefined, so when trying to access pageResources.page it crashes.

Playing around with the loaders in the windowI found that loadPageSync returns undefined if the page has not been fetched before, if I fetch it with loadPage then loadPageSync returns the data without problems:

image

this loadPageSync undefined on the first call doesn't happen when not using the plugin:
image

this is same issue as #10 and #5

@wardpeet any ideas?

@blessanm86
Copy link
Author

@apaniel90vp
I have been trying to figure whats going wrong and I modified the condition on this line to

if (pagePath && path === location.pathname) {

this seems to fix the exception. but the page still navigates away when the browser url and the gatsby url are different

@apaniel
Copy link
Contributor

apaniel commented Sep 5, 2019

thanks @Blessenm! I think that what you found is another issue, window.pagePath happens to be undefined when running gatsby develop
I am testing everything with gatsby build --prefix-paths, and window.pagePath is not undefined when building for prod, so that doesn't fix the issue.

I think that there must be some problem with the prefetch. that loadPageSync is trying to get the page before prefetch is executed, I am still investigating

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