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

fix: no pathname setter in some contexts, set during construction #8861

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mikehardy
Copy link

Discussion

Hi there! 👋 I'm part of the team that maintains react-native-firebase and we've been using the rules-unit-testing package for a long time, but we've also been carrying around a local patch for it for a long time

Our problem is that in some contexts (react-native in particular) URL.pathname has a getter but not a setter and we run in strict mode, so attempting to assign to URL.pathname results in a runtime error. I understand (via a quick check in jsfiddle) that even with "use strict"; in other contexts this is not a problem, their URL implementations must differ, but in react-native at least it is an immediate error for us that we must patch

     TypeError: Cannot assign to property 'pathname' which has only a getter
      at makeUrl (/Users/mike/work/invertase/react-native-firebase/tests/node_modules/@firebase/rules-unit-testing/dist/index.cjs.js:64:16)
      at loadDatabaseRules (/Users/mike/work/invertase/react-native-firebase/tests/node_modules/@firebase/rules-unit-testing/dist/index.cjs.js:384:23)

The path is known in advance though, so it may simply be passed in the URL constructor for a functionally equivalent effect, with a very tiny, hopefully visually obviously correct diff. This diff works in our testing (and has in CI for a year or so)

Hopefully we can upstream this small change and drop our patch locally

Thanks for your consideration

Testing

  • Make sure all existing tests in the repository pass after your change.
  • If you fixed a bug or added a feature, add a new test to cover your code.

API Changes

  • At this time we cannot accept changes that affect the public API. If you'd like to help
    us make Firebase APIs better, please propose your change in an issue so that we
    can discuss it together.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
In some contexts (react-native in particular) URL.pathname has a getter but not a setter, so attempting to assign to URL.pathname results in a runtime error.

The path is known in advance though, so it may simply be passed in the URL constructor for a functionally equivalent effect
@mikehardy mikehardy requested review from avolkovi, sam-gc, yuchenshi and a team as code owners March 24, 2025 14:46
Copy link

changeset-bot bot commented Mar 24, 2025

⚠️ No Changeset found

Latest commit: 7ef4c00

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

mikehardy added a commit to invertase/react-native-firebase that referenced this pull request Mar 24, 2025
works in testing, is minimal compared to previous

See firebase/firebase-js-sdk#8861
mikehardy added a commit to invertase/react-native-firebase that referenced this pull request Mar 27, 2025
works in testing, is minimal compared to previous

See firebase/firebase-js-sdk#8861
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.

None yet

1 participant