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

feat(origin): set to optional #18

Closed
wants to merge 0 commits into from

Conversation

TartanLeGrand
Copy link
Contributor

Overview 🌐

This PR introduces a significant enhancement to our Nuxt project by making the configuration of the origin property optional. This change leverages the capabilities of Nuxt's useRequestUrl from the Nuxt Documentation to dynamically determine the origin URL. This update aims to streamline configuration and improve the flexibility of our application.

Changes 🛠️

  • Optional Origin Configuration: The origin configuration in our Nuxt setup has been modified to be optional. This change is intended to reduce the manual setup required and make our application more adaptive to different environments.

  • Dynamic Origin Retrieval: Integrated Nuxt's useRequestUrl to automatically fetch the origin URL. This approach ensures that our application can dynamically adjust to the environment it is running in, whether in development, staging, or production.

Implementation Details 📝

  • Modified the configuration file to check if the origin is provided. If not, useRequestUrl is utilized to derive the origin.
  • Ensured backward compatibility so that existing configurations with a defined origin continue to function seamlessly.
  • Added relevant unit tests to cover the new scenarios introduced by these changes.

How to Test 🧪

  • With Origin Configured: Set up the application with the origin configured and ensure the application behaves as expected.
  • Without Origin Configured: Remove the origin configuration and observe if the application correctly determines the origin using useRequestUrl.

Benefits ✨

  • Simplicity: Reduces the complexity of initial setup and configuration.
  • Flexibility: Increases the adaptability of our application across different environments.
  • Maintenance: Eases future maintenance by relying on built-in Nuxt functionality.

Request for Comments 🗣️

I encourage the team to review these changes and provide feedback. Any suggestions for further improvements or concerns are highly welcomed. Let's discuss how we can make this integration as smooth and beneficial as possible!

Copy link
Owner

@manchenkoff manchenkoff left a comment

Choose a reason for hiding this comment

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

Hey @TartanLeGrand, thanks a lot for the contribution! Left some comments, pls take a look when you have time!

src/runtime/httpFactory.ts Outdated Show resolved Hide resolved
src/runtime/httpFactory.ts Outdated Show resolved Hide resolved
@manchenkoff
Copy link
Owner

There is still some formatting issue during the build, @TartanLeGrand could you please run yarn fmt and yarn lint locally and fix the issue?

@TartanLeGrand
Copy link
Contributor Author

There is still some formatting issue during the build, @TartanLeGrand could you please run yarn fmt and yarn lint locally and fix the issue?

It's good

@TartanLeGrand
Copy link
Contributor Author

@manchenkoff but the build still broken because you have an error on your main branch. The types are not resolve.

@manchenkoff
Copy link
Owner

Hey @TartanLeGrand, it's not in main branch, here is the latest build. Build here is complaining about your changes with origin type

@TartanLeGrand
Copy link
Contributor Author

Hello, no. Have try on your main branch 😄
image

@manchenkoff
Copy link
Owner

@TartanLeGrand yes, the build I shared with you was executed on main branch in the repo. You can check commit hash in Checkout step - 65e865ecc905f3a1a86bda65fdadb47c16f5cbfc

@manchenkoff
Copy link
Owner

manchenkoff commented Jan 2, 2024

I assume it's because of the line 57 in src/runtime/httpFactory.ts since you use origin which is now string | nullish instead of string | undefined.

@manchenkoff
Copy link
Owner

@TartanLeGrand thanks again 👍 merged and published as v0.0.16

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

Successfully merging this pull request may close these issues.

2 participants