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: robust handling for internal uri #111

Open
wants to merge 29 commits into
base: develop
Choose a base branch
from

Conversation

SH4LIN
Copy link
Collaborator

@SH4LIN SH4LIN commented Mar 2, 2025

What

  • This PR will allow developers to individually specify the home URL and site URL.
  • This PR will add wrapper functions to identify internal URLs more robustly
  • This PR will replace the internal URL with the NextJS app URL using JS URL API instead of simple string replace.

Why

  • Current approach for the replace is prone to bugs.

Related Issue(s):

How

  • We will parse the URL using the URL JS API and replace the properties with the nextApp url properties and build the URL again from it.

Checklist

  • I have read the Contribution Guidelines.
  • My code is tested to the best of my abilities.
  • My code passes all lints (ESLint, tsc, prettier etc.).
  • My code has detailed inline documentation.
  • I have added unit tests to verify the code works as intended.
  • I have updated the project documentation as needed.
  • I have added a changeset for this PR using npm run changeset.

SH4LIN added 2 commits March 2, 2025 23:00
…environment variable to allow developers to specify the home URL and site URL individually.
# Conflicts:
#	packages/core/src/config/snapwp-config-manager.ts
Copy link

changeset-bot bot commented Mar 2, 2025

🦋 Changeset detected

Latest commit: 7dc9e5b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@snapwp/blocks Major
@snapwp/query Minor
@snapwp/types Minor
@snapwp/core Minor
@snapwp/next Major
snapwp Minor
@snapwp/codegen-config Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@SH4LIN SH4LIN requested review from justlevine and ayushnirwal March 3, 2025 06:22
@SH4LIN SH4LIN marked this pull request as ready for review March 3, 2025 08:41
@SH4LIN SH4LIN requested review from justlevine and ayushnirwal March 3, 2025 08:55
ayushnirwal
ayushnirwal previously approved these changes Mar 4, 2025
Copy link
Collaborator

@ayushnirwal ayushnirwal left a comment

Choose a reason for hiding this comment

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

LGTM.

The env vars for CI/CD are needed to be updated.

@SH4LIN
Copy link
Collaborator Author

SH4LIN commented Mar 4, 2025

@justlevine This PR is up for re-review.

@justlevine
Copy link
Collaborator

(@justlevine audit env var names)

@justlevine justlevine added the needs: review 🧐 Needs to be reviewed by a codeowner label Mar 4, 2025
@justlevine justlevine self-assigned this Mar 4, 2025
@justlevine
Copy link
Collaborator

(Also, dont forget to add a changelog)

@justlevine justlevine removed the needs: review 🧐 Needs to be reviewed by a codeowner label Mar 4, 2025
@justlevine justlevine assigned SH4LIN and unassigned justlevine Mar 4, 2025
…to to-frontend-uri.ts, update logic for isInternalUrl
@SH4LIN SH4LIN requested a review from ayushnirwal March 11, 2025 11:00
@SH4LIN SH4LIN requested a review from justlevine March 14, 2025 12:15
Copy link
Collaborator

@justlevine justlevine left a comment

Choose a reason for hiding this comment

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

These are comments from an old code review. Not sure why they didn't get shared w. The rest, nor if they're still accurate 🤦‍♂️

I'll re-rereview tomorrow night

@SH4LIN SH4LIN requested a review from justlevine March 14, 2025 13:16
Copy link
Collaborator

@justlevine justlevine left a comment

Choose a reason for hiding this comment

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

Last round of feedback from me (hopefully)

If @ayushnirwal has no suggestions for a utils naming convention, then I'll see what GPT recommends

@SH4LIN SH4LIN requested a review from justlevine March 19, 2025 10:05
Copy link
Collaborator

@justlevine justlevine left a comment

Choose a reason for hiding this comment

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

@Ta5r can you PTAL at the changes to the env variables (both documented and the comments that get output in the .env.example), and make a matching PR for snapwp-helper?

@justlevine justlevine self-requested a review March 19, 2025 22:01
@justlevine justlevine added the status: hold ⏸️ Needs other items to be resolved before work can continue label Mar 19, 2025
Copy link
Collaborator

@justlevine justlevine left a comment

Choose a reason for hiding this comment

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

Can be merged after @Ta5r updates the env vars in rtcamp/snapwp-helper

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: hold ⏸️ Needs other items to be resolved before work can continue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants