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

Allow CORS mode to be optional #60

Closed
wants to merge 3 commits into from
Closed

Conversation

jakemwood
Copy link

Hello! We are trying to use the Webflow API with Cloudflare Workers. Unfortunately, their fetch API does not allow the usage of cors mode. This seems to make sense, since there shouldn't be any cross-origin issues in a Node.JS environment, as there is no origin. However, I have added an optional parameter to the Webflow constructor (useCors) to ask if the user would like to use CORS or not. It defaults to true to make this a non-breaking change. I've also updated the TypeScript definition to match.

src/Webflow.js Outdated
@@ -52,7 +52,7 @@ const responseHandler = (res) =>
});

export default class Webflow {
constructor({ endpoint = DEFAULT_ENDPOINT, token, version = "1.0.0" } = {}) {
constructor({ endpoint = DEFAULT_ENDPOINT, token, version = "1.0.0", useCors = true } = {}) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should be a generic options = {} overload that the caller can use to provide anything to fetch 🤔 It might future proof it for other factors not considered.

Copy link
Author

@jakemwood jakemwood Aug 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @johnagan! Good idea. I've made the change but called it fetchOpts to be a bit more specific.

I think the only unfortunate side-effect to this approach is that CORS specifically requires you to use undefined, which is maybe unintuitive. I've added an example to the readme, though, to help offset this concern.

@johnagan
Copy link
Contributor

johnagan commented Aug 5, 2022

Thanks @jakemwood! I'm aligned with getting a fix for you. Made a suggestion on approach - let me know your thoughts.

@@ -76,6 +76,7 @@ export default class Webflow {
method,
headers: this.headers,
mode: "cors",
Copy link
Contributor

@johnagan johnagan Aug 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left over from the original PR

@@ -24,6 +24,9 @@ const Webflow = require('webflow-api');
// Initialize the API
const api = new Webflow({ token: 'api-token' });

// Initialize the API without CORS mode
const apiNoCors = new Webflow({ token: 'api-token', fetchOpts: { mode: undefined }});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is left over as well

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is a leftover. This is how you would accomplish the removal of cors mode with the fetch options override approach you suggested instead of the useCors: false approach from the first pass.

@@ -76,6 +76,7 @@ export default class Webflow {
method,
headers: this.headers,
mode: "cors",
...fetchOpts,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing about this approach is it would override the headers (including auth) if someone added a header entry.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm. Could change the order:

mode: "cors",
...fetchOpts,
method,
headers: this.headers,

I think this would do the trick. Thoughts?

@johnagan
Copy link
Contributor

You're welcome to pushback on this, but my recommendation is to make a PR to override fetch options rather than focusing on disabling cors.

Client Code

so I'd imagine something like this:

const options = {
  mode: undefined,   // this is your use-case, but likely an edge case
  headers: {
    "User-Agent": "My Awesome Webflow App"    // a more likely use-case for this feature
  }
};

const webflow = new Webflow({ token: 'api-token', options });

Changes to SDK

Possibly add something like this to line 55.

constructor({ endpoint = DEFAULT_ENDPOINT, token, version = "1.0.0", options = {} } = {}) {
  if (!token) throw buildRequiredArgError("token");

  this.responseWrapper = new ResponseWrapper(this);

  this.endpoint = endpoint;
  this.token = token;
  
   // Enable header overrides
  this.headers = Object.assign({
    Accept: "application/json",
    Authorization: `Bearer ${token}`,
    "accept-version": version,
    "Content-Type": "application/json"
  }, options.headers);

  this.authenticatedFetch = (method, path, data, query) => {
    const queryString =
      query && !isObjectEmpty(query) ? `?${qs.stringify(query)}` : "";

    const uri = `${this.endpoint}${path}${queryString}`;
    const opts = {
      method,
      mode: "cors",
      ...options,
      headers, // set headers after blanket override
    };

    if (data) {
      opts.body = JSON.stringify(data);
    }

    return fetch(uri, opts).then(responseHandler);
  };
}

I'm winging it on coding in comments here, but let me know what you think about this approach!

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

Successfully merging this pull request may close these issues.

2 participants