Skip to content

Firestore: Hardcode path names in rollup scripts #7953

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dconeybe
Copy link
Contributor

@dconeybe dconeybe commented Jan 16, 2024

This PR modifies Firestore's rollup scripts to hardcode the paths of the input and output files, rather than loading them from package.json. No functionality whatsoever was changed. The motivation for this change was to improve readability of the rollup scripts: with the paths hardcoded it is much easier to understand from reading the scripts which files are getting consumed and which are getting created.

Work In Progress

This PR is a work-in-progress. As of Jan 22, 2024, work on this PR has been de-prioritized but I'm leaving it here so it can be picked up in the future. Googlers see b/302682954 for details.

@dconeybe dconeybe self-assigned this Jan 16, 2024
Copy link

changeset-bot bot commented Jan 16, 2024

⚠️ No Changeset found

Latest commit: fd3855d

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

@google-oss-bot
Copy link
Contributor

@google-oss-bot
Copy link
Contributor

@dconeybe dconeybe marked this pull request as ready for review January 16, 2024 15:17
@dconeybe dconeybe requested review from a team as code owners January 16, 2024 15:17
Copy link
Contributor

@MarkDuckworth MarkDuckworth left a comment

Choose a reason for hiding this comment

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

I'm leaning towards the risks outweighing the readability improvement. Two downsides. First, I think this makes the code more fragile, requiring changes to the paths to be made consistently in multiple places in code. Second, this makes Firestore inconsistent with other packages. The inconsistency makes it more likely that a Firebase maintainer would not modify all appropriate strings. I'm open to discussing.

@dconeybe
Copy link
Contributor Author

I'm leaning towards the risks outweighing the readability improvement. Two downsides. First, I think this makes the code more fragile, requiring changes to the paths to be made consistently in multiple places in code. Second, this makes Firestore inconsistent with other packages. The inconsistency makes it more likely that a Firebase maintainer would not modify all appropriate strings. I'm open to discussing.

Thank you for the input, @MarkDuckworth. Based on these comments I'm trying a different approach that meets somewhere in the middle. I'll let you know once that's ready.

@dconeybe
Copy link
Contributor Author

@MarkDuckworth How do you feel about this solution instead: #7956

Basically, it puts the logic to extract the paths from package.json into a helper function so that we don't have all these bespoke mechanisms for doing so scattered throughout the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants