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

chore: merge url options #16

Merged
merged 8 commits into from
Mar 10, 2022
Merged

chore: merge url options #16

merged 8 commits into from
Mar 10, 2022

Conversation

james-deepsource
Copy link
Contributor

@james-deepsource james-deepsource commented Mar 9, 2022

Description

This PR aims at unifying the development and production WebSocket connection URLs into a single option and other improvements.

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Changes in this PR

  • Remove urlForDev and urlForProd options.
  • Add url option.
  • Specify WebSocket URL via runtime config with websocket key.
// nuxt.config.js
export default {
  // Via Runtime config
  publicRuntimeConfig: {
    websocket: {
      url: process.env.WEBSOCKET_URL
    }
  }
};
  • Update docs to reflect the changes.
  • Update test suite.
  • Update references websocket -> WebSocket.

Checklist before merging

  • Tests
  • Code and Readme documentation

Note to reviewers (Optional)

As suggested by @shivam-deepsource

@james-deepsource james-deepsource changed the title Update options chore: merge url options Mar 9, 2022
test/plugin.test.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@shivam-deepsource shivam-deepsource left a comment

Choose a reason for hiding this comment

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

I've added suggestions for the README as a separate PR to this branch, check it out here

Also, can we add validation for the URL, instead of having a default to an external URL, let's disable the plugin if the URL is not present. We can do it in a separate PR

@yash-deepsource yash-deepsource dismissed their stale review March 10, 2022 09:32

pending changes

* feat: update readme

* chore: update readme title

Co-authored-by: James George <[email protected]>

Co-authored-by: James George <[email protected]>
@james-deepsource james-deepsource force-pushed the update-options branch 2 times, most recently from 5bb869d to 103de9d Compare March 10, 2022 10:02
README.md Outdated Show resolved Hide resolved
Co-authored-by: Yash Dave <[email protected]>
@james-deepsource
Copy link
Contributor Author

@shivam-deepsource PTAL.

@james-deepsource james-deepsource merged commit dfff575 into main Mar 10, 2022
@james-deepsource james-deepsource deleted the update-options branch March 10, 2022 12:10
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.

3 participants