-
Notifications
You must be signed in to change notification settings - Fork 70
feat: support for a custom OpenNext config path #862
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
Conversation
🦋 Changeset detectedLatest commit: 2da636d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me. Thanks!
One question: This PR uses a relative path. Quickly looking at the code, it looks like an absolute path would work. If that is the case, can you please update the code (and maybe aws). If absolute paths do not work, we probably should add a check to fail early.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's discuss the open point in opennextjs/opennextjs-aws#972 before we merge this
d44ce85
to
afd7dc2
Compare
@james-elicx could you please update the PR description to include:
We can keep #871 around until we remove Thanks! |
Yes, that's a dependency for absolute paths to work with the cloudflare cli. Updated the PR desc. |
078e979
to
f08b19f
Compare
f08b19f
to
4c798da
Compare
@james-elicx I have added a proposal as a fixup commit. Main changes:
Looking at the comments/changes in What do you think? |
This is why I used to keep asking when we'd get a monorepo :( Personally, I prefer not adding hacks simply because we're using polyrepos, even if that means it might take longer to ship something.
Makes sense, looks fine. |
Thanks for working on this James!
It's not always easy to balance priority across features, fixes, customer wishes, and resources. The hack is tested and should not be here for long so I think it's safe to merge this for now. As for the mono-repo, definitely something we should think about before jumping into the Adapters API effort. |
Changes
--openNextConfigPath
cli flag that lets you use a custom config path (e.g. if you had a cli that internally called opennext, you could have your own build dir have the config path instead of it leaking out).--configPath
flag for wrangler configs, in favour of--config
.wranglerConfigPath
for clarity.depends on opennextjs/opennextjs-aws#972 for absolute paths.
Note: both
--openNextConfigPath
and--config
treat path starting with/
as absolute paths.outcome of #853