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] Re-use loadEnvFiles from cli-helpers lib, elimininating code duplicatation #11885

Merged
merged 5 commits into from
Jan 8, 2025

Conversation

Philzen
Copy link
Contributor

@Philzen Philzen commented Jan 7, 2025

While investigating for #11882 i stumbled over the fact that those lib files were almost identical twins. Unless there is a very, very good reason to have both around this PR determines the single-source-of-truth to be in @redwood/cli-helpers and drops the other one, effectively reducing our technical debt here.

No need to preserve the file in the cli package, as it had an identical git history up until 36fa1dd, which was 10 months ago. Since then, the version in @redwood/cli-helpers had four more commits, so it should be the better version.

This is also a good preparation for further investigation into (and hopefully a timely fix for) #11884.

@Philzen Philzen changed the title Re-use loadEnvFiles from cli-helpers and delete effective duplicate [chore] Re-use loadEnvFiles from cli-helpers lib and delete effective duplicate Jan 7, 2025
@Tobbe Tobbe added release:chore This PR is a chore (means nothing for users) changesets-ok Override the changesets check labels Jan 7, 2025
@Tobbe Tobbe added release:breaking This PR is a breaking change and removed release:chore This PR is a chore (means nothing for users) labels Jan 7, 2025
@Tobbe
Copy link
Member

Tobbe commented Jan 7, 2025

People are deep-importing from our packages. So when we remove something like this we need to make a note of it in our release notes. It sucks this has to be marked as a breaking change. And no one is probably actually importing some random cli export... But it is what it is...

@Tobbe Tobbe removed the changesets-ok Override the changesets check label Jan 7, 2025
No need to preserve the file in the cli package, as it had an identical
git history up until 36fa1dd, which
was 10 months ago. Since then, the version in @redwood/cli-helpers had
four more commits, so it should be the better version.
@Philzen Philzen force-pushed the refactor/unify-loadEnvFiles-codebase branch from 81bbc1c to 926b6c6 Compare January 7, 2025 17:43
@Philzen
Copy link
Contributor Author

Philzen commented Jan 7, 2025

has to be marked as a breaking change.

So you are saying that people are deep-importing from @redwood/cli?!? In a typical project i cannot even access that package 🤔

What i can do though is explicitly install @redwood/cli and then, yes i can do import { loadEnvFiles } from '@redwoodjs/cli/dist/lib/loadEnvFiles'.

My suggestion for this PR would therefore simply be to keep the file as a stub that just re-exports it. So it won't be a breaking change, right?

This PR would then be compatible for release in a non-major version bump, and we can do a separate one that simply removes that stub.

@Philzen Philzen marked this pull request as draft January 7, 2025 17:55
@Philzen Philzen force-pushed the refactor/unify-loadEnvFiles-codebase branch from 3ba27b2 to 5686f64 Compare January 7, 2025 18:06
@Philzen
Copy link
Contributor Author

Philzen commented Jan 7, 2025

People are deep-importing from our packages. It sucks this has to be marked as a breaking change.

@Tobbe Problem avoided via 5686f64

So when we remove something like this we need to make a note of it in our release notes.

When this is merged, it's as simple as reverting that commit in a separate PR which will be the breaking change for the next major.

@Philzen Philzen marked this pull request as ready for review January 7, 2025 18:09
@Philzen Philzen changed the title [chore] Re-use loadEnvFiles from cli-helpers lib and delete effective duplicate [chore] Re-use loadEnvFiles from cli-helpers lib, eliminiating code duplicatation Jan 7, 2025
@Philzen Philzen changed the title [chore] Re-use loadEnvFiles from cli-helpers lib, eliminiating code duplicatation [chore] Re-use loadEnvFiles from cli-helpers lib, elimininating code duplicatation Jan 7, 2025
Philzen added a commit to Philzen/redwood that referenced this pull request Jan 7, 2025
@Tobbe Tobbe added release:chore This PR is a chore (means nothing for users) and removed release:breaking This PR is a breaking change labels Jan 8, 2025
@Tobbe Tobbe added the changesets-ok Override the changesets check label Jan 8, 2025
@Tobbe Tobbe merged commit f66ca2e into redwoodjs:main Jan 8, 2025
57 of 61 checks passed
@Philzen Philzen deleted the refactor/unify-loadEnvFiles-codebase branch January 8, 2025 05:17
@Philzen Philzen restored the refactor/unify-loadEnvFiles-codebase branch January 8, 2025 05:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changesets-ok Override the changesets check release:chore This PR is a chore (means nothing for users)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants