Skip to content
This repository was archived by the owner on Jun 13, 2022. It is now read-only.

feat: adding corsOptions config variable for CORS middleware in app.js #429

Closed
wants to merge 1 commit into from

Conversation

tech4242
Copy link

@tech4242 tech4242 commented Nov 19, 2019

Summary

The idea of this PR is simple - introduce an optional corsOptions parameter to the siteConfig, so that one can e.g. create an Angular/React/... app where the Statusfy API is reachable without the typical CORS header errors in modern browsers like Chrome. If corsOptions is not set, nothing changes to the CORS middleware settings in Express. I have also added this to the Configuration documentation with a link to the official Express documentation. Since there is no data where authentication is needed, I thought it's harmless to give '*' as the parameter for origin but made a note for users to check out the official documentation on the middleware if they have not used it before. No tests were written for this feature.

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Docs
  • Build-related changes
  • Other, please describe:

If changing the UI of default theme, please provide the before/after screenshot:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)

You have tested in the following browsers: (Providing a detailed version will be better.)

  • Chrome
  • Firefox
  • Safari
  • Edge
  • IE

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature
  • Related documents have been updated
  • Related tests have been updated

@welcome
Copy link

welcome bot commented Nov 19, 2019

Thanks so much for opening your first pull request! Please check out our contributing guidelines.

@ghost
Copy link

ghost commented Nov 19, 2019

There were the following issues with this Pull Request

  • Commit: 23e12fa
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: 384680a
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@CLAassistant
Copy link

CLAassistant commented Nov 19, 2019

CLA assistant check
All committers have signed the CLA.

@ghost
Copy link

ghost commented Nov 19, 2019

There were the following issues with this Pull Request

  • Commit: 23e12fa
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: 384680a
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: 3e7f553
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@tech4242 tech4242 force-pushed the tech4242-cors-route branch from 3e7f553 to 2de4480 Compare November 20, 2019 09:05
@tech4242 tech4242 changed the title Adding optional CORS settings to app.js feat: adding corsOptions config variable for CORS middleware in app.js Nov 20, 2019
@juliomrqz juliomrqz requested a review from giovagnoli November 21, 2019 14:32
@juliomrqz juliomrqz requested review from juliomrqz and a user and removed request for giovagnoli November 21, 2019 14:32
@juliomrqz juliomrqz added enhancement New feature or request pinned Important Issues labels Nov 21, 2019
@tech4242
Copy link
Author

tech4242 commented Nov 26, 2019

If you need anything from me to move forward with the PR, please let me know @juliomrqz

@ghost
Copy link

ghost commented Dec 6, 2019

There were the following issues with this Pull Request

  • Commit: 6206bee
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@tech4242 tech4242 force-pushed the tech4242-cors-route branch from 6206bee to bdfaa76 Compare December 9, 2019 09:22
@tech4242
Copy link
Author

tech4242 commented Dec 9, 2019

The CI is failing because of your linter about changes that I have not committed. It initially complained about my use of !! for my CORS statement in app.js but I fixed that. Now it has found 16 other errors that were not caused by my commits (see Travis logs). Please let me know how we can proceed on this PR.

@juliomrqz
Copy link
Owner

juliomrqz commented May 20, 2020

Hello @tech4242. There are big changes coming (mostly internal) for the next major version of Statusfy (v1). For more details go to #551. A CORS middleware will not be required with the new changes

We appreciate your comments or concerns.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request pinned Important Issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants