wpengine / faustjs

Faust.js™ - The Headless WordPress Framework
https://faustjs.org
Other
1.44k stars 134 forks source link

Bug: Endless refreshing when trying to preview post with different basePath in Next.js config #1287

Closed riderjensen closed 1 year ago

riderjensen commented 1 year ago

When adding a basePath to next.config.js (for example /blog), the preview mode gets stuck in an endless authentication loop.

Applicable Versions

Steps To Reproduce

  1. Create a base wordpress instance with faust/gql/etc
  2. Create a faust project with base config for next.js
  3. In the next.config.js add a property called basePath and set it to /blog
  4. In wordpress, in your Faust settings, set Front-end site URL to reflect the basePath (i.e. http://localhost:3000/blog)
  5. Attempt to preview one of the posts
  6. Note the page never loads and refreshes over and over again with a different code query parameter

Link to code example:

While I don't have a code sample, I can point out the reason this is happening:

  1. Observe that the FAUST_API_BASE_PATH is hardcoded within the libs here
  2. Observe that the FAUST_API_BASE_PATH is imported and used within the router here
  3. Observe that the FAUST_API_BASE_PATH is imported and used within the accessToken.ts file here
  4. Note that there is no way to change this constant in any config
  5. It should be noted that the docs for trying to change the apiBasePath do not apply here as you can see the config takes no parameter for apiBasePath
  6. accessToken.ts is called by authorize.ts which is called by WordPressTemplate on mount.
  7. Observe that the url in fetchAccessToken located in accessToken.ts is made up of constant values that cannot be change but that do also not reference the potential for a Next.js basePath to have been aded.
  8. Therefore, even when my base URL is http://localhost:3000/blog, accessToken requests will be directed to http://localhost:3000/api/faust/auth/token instead of http://localhost:3000/blog/api/faust/auth/token which will 404
  9. Because ensureAuthorization function takes a parameter of redirectUri which is the same location as where we are now, the page essentially refreshes and repeats this over and over.
Screenshot 2023-02-22 at 12 22 46 PM

The current behavior

The page constantly redirects and 404s over and over again

The expected behavior

The preview page should load

riderjensen commented 1 year ago

Suggested Solution To Above Problem

The constant refreshing is just a symptom, not actually the cause of this issue. The real cause is the basePath not being considered in the authorization call our API. Therefore, suggested solution is: Extend FaustConfig to include apiBasePath. Then when we reference FAUST_API_BASE_PATH in authorize.ts, we first get the config and check if apiBasePath is set. If it is, we use the user config value. If not, we fallback to the default constant.

After Implementing Above Solution Locally:

A second related issue was found. You can see on the WordPressTemplate.tsx that we check if its a preview and then pull off the url from the parameter previewPathname. previewPathname is provided from wordpress when you click the "Preview" button the wordpress UI. However, there is no filtering on the Wordpress side from which I can tell. This can be fixed by a string replace but I am not convinced its the best way to continue and I am still investigating potential solution suggestions. Im hopeful this specific issue could be a misconfiguration in WP instead of a Faust issue like the bug listed above

Sample solution for seedQueryUri:

        const { basePath } = getConfig();
        seedQueryUri = getQueryParam(window.location.href, 'previewPathname');
        if (basePath) {
          seedQueryUri = seedQueryUri.replace(basePath, '');
        }
riderjensen commented 1 year ago

Ill have an MR open by the end of week with my solution that I will link here. Mostly inspired by the deprecated core package (maybe this functionality wasn't migrated on purpose?)

theodesp commented 1 year ago

Hey @riderjensen. Thank you very much for reporting this. It looks like we didn't port this feature. Any PRs would be welcome.

As for the previewPathname I think we need to revisit this piece pf code to see if we can improve it.

riderjensen commented 1 year ago

@theodesp Thanks for letting me know, I have created a PR.

Another comment that could come up later to hurt the project is that the router parsedUrl is for the most part undefined. The req.url coming in looks like '/api/auth/faustinstead of includinghttp://example.com`. Not sure if this warrants it's own issue but I thought I would bring it up as its semi-related.

blakewilson commented 1 year ago

Hey @riderjensen, thanks for the PR, this is great! We'll get this reviewed soon.

As for the req.url being relative, this is standard in Next.js/Express, etc. so not much can be done there.

theodesp commented 1 year ago

Closing this one since it was merged

austintreneff commented 1 year ago

Hey @theodesp @blakewilson we are still experiencing this issue with previews resulting in an infinite redirect on Faust 1.0.1. It's still trying to authenticate at /api/faust instead of /blog/api/faust

I have added the basePath: '/blog' to my next.config.js

austintreneff commented 1 year ago

I am able to reproduce the issue as outlined above, using the faust getting started template @theodesp @blakewilson

theodesp commented 1 year ago

@austintreneff did you set apiBasePath in faust.config.js as well? This needs to match the basePath parameter.

austintreneff commented 1 year ago

@theodesp apiBasePath does not seem to be a supported parameter in faust.config.js

However, I have tried adding apiBasePath to both the faust.config.js and the next.config.js

Both my basePath and apiBasePath are set to /blog but the authentication is still trying to authenticate at /api/faust

Is there an example of this working I could take a look at?

perrywortman commented 1 year ago

Thanks @austintreneff, I am also able to repro. Possible cause:

Looks like we are calling getConfig and destructuring basePath at the top of packages/faustwp-core/src/auth/client/accessToken.ts before the config actually has a chance to load. Because of that basePath will be undefined since we will be using the defaults from normalizeConfig in packages/faustwp-core/src/config/index.ts.

To prevent this from happening, we can just move the call to getConfig and ref to basePath inside of fetchAccessToken:

export async function fetchAccessToken(code?: string): Promise<string | null> {
  const { basePath } = getConfig();
  let url = `${ basePath ?? ''}${FAUST_API_BASE_PATH}/${TOKEN_ENDPOINT_PARTIAL_PATH}`;
  ...
}

Happy to submit a PR for these changes. Does this sound reasonable to all? Or did I miss something (which is very possible)?

cc: @theodesp @blakewilson

theodesp commented 1 year ago

@perrywortman sure. Feel free to submit a PR for this. Thank you.

perrywortman commented 1 year ago

Hi @theodesp. I was just playing around with the experimental toolbar locally and noticed that the logout feature was 404ing. Seemed related to the basePath issue so I went ahead and prefixed all instances of api routes with the basePath and it fixed this issue as well. It's also possible that this issue might be related and solved by this fix.

I just updated my PR with the more comprehensive fix here and a new changeset. Let me know what you think - thanks and have a great weekend!

perrywortman commented 1 year ago

Just checking in on this thread again after a couple of weeks - looks like my PR still needs a review and some of the jobs are failing. Please let me know if there is anything I can do to help push things along.

cc: @theodesp @blakewilson

josephfusco commented 1 year ago

@riderjensen @perrywortman Thank you for of your work with this! That PR has been merged and is available to test in the latest canary release.

npm i @faustwp/core@canary

Please let us know if that resolves the original issue!

perrywortman commented 1 year ago

Thanks! Sounds good @josephfusco - we started testing and will keep you posted sometime today or tomorrow.

perrywortman commented 1 year ago

Hi @josephfusco

After testing the Dashlane blog with @faustwp/core@canary and 1.0.3 of the WP plugin, we are seeing 404 errors whenever api routes are hit, e.g. previews. The specific error I see is unexpected end of json input from the fetching of the access token in /auth/client/accessToken.ts

However, if I manually apply my changes to the latest version @faustwp/core@1.1.0 everything works as expected. Any idea why this 404 might happen only on canary?

mindctrl commented 1 year ago

@perrywortman thank you for the update. We'll take a closer look.

josephfusco commented 1 year ago

The endless refreshing has been patched in canary. Please let us know if that resolves things for you!

npm i @faustwp/core@canary
blakewilson commented 1 year ago

Thanks @josephfusco, I was able to confirm this is working as expected with basePath specified and using the latest canary version of @faustwp/core 🎉

https://github.com/wpengine/faustjs/assets/5946219/b056631e-8675-42a8-a012-2e4c992885d3

perrywortman commented 1 year ago

@blakewilson @josephfusco Confirmed working on my end as well. Thank you!