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

refactor: project options type shared with build + config init #88

Merged
merged 2 commits into from
Oct 11, 2024

Conversation

james-elicx
Copy link
Collaborator

@james-elicx james-elicx commented Oct 10, 2024

Context

We have what seems to be a pre-options and then options. It would help in the long-term to have a single set of options that we use to stub the project setup and the config creation, especially if we add more CLI options in the future. Therefore, the types are slightly refactored to share the same thing between the creation of the project config and the project setup function.

I don't mind if you want to keep it as is, but I think it would be simpler to have a single object used between the two.

I wanted to move the config creation to before the project setup outside but it currently depends on the build happening in order to generate the config unfortunately for the .next server directory.

Changes

  • Shares the same input between the two.
  • Renames builderOutput to outputDir and nextApp to sourceDir, as it's the output directory and the source directory.

basically centralises where i need to make a change if i want to add a new cli arg (e.g. #89) and use it in the config, otherwise you need to update multiple types and args normally

Copy link

changeset-bot bot commented Oct 10, 2024

⚠️ No Changeset found

Latest commit: 0d3b69d

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

Copy link

pkg-pr-new bot commented Oct 10, 2024

Open in Stackblitz

pnpm add https://pkg.pr.new/@opennextjs/cloudflare@88

commit: 0d3b69d

@james-elicx james-elicx force-pushed the james/refactor-config-build branch 5 times, most recently from 2255cfe to 7570200 Compare October 10, 2024 21:39
@james-elicx james-elicx changed the title refactor: pass config to build function refactor: project options type shared with build + config init Oct 10, 2024
@james-elicx james-elicx force-pushed the james/refactor-config-build branch from 7570200 to 24ec9c0 Compare October 10, 2024 21:42
@james-elicx james-elicx force-pushed the james/refactor-config-build branch from 24ec9c0 to cc44647 Compare October 10, 2024 21:43
@james-elicx james-elicx marked this pull request as ready for review October 10, 2024 21:47
Copy link
Contributor

@vicb vicb left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, I added a few minor comments.

For the next time please do me a favor and split changes in multiple commits. For example the first commit should have been the config refactoring and the second the renaming which would be trivial to review. It would be much faster to review.

packages/cloudflare/src/cli/build/index.ts Outdated Show resolved Hide resolved
packages/cloudflare/src/cli/config.ts Outdated Show resolved Hide resolved
packages/cloudflare/src/cli/config.ts Outdated Show resolved Hide resolved
@james-elicx
Copy link
Collaborator Author

For the next time please do me a favor and split changes in multiple commits. For example the first commit should have been the config refactoring and the second the renaming which would be trivial to review. It would be much faster to review.

Ah sorry about that, totally fair point - will do in the future.

@james-elicx james-elicx requested a review from vicb October 11, 2024 06:59
Copy link
Contributor

@vicb vicb left a comment

Choose a reason for hiding this comment

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

Thanks!

@vicb vicb merged commit 450efc7 into main Oct 11, 2024
7 checks passed
@vicb vicb deleted the james/refactor-config-build branch October 11, 2024 07:05
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.

2 participants