refactor: compileOpenNextConfig now take a path to the config file

[james-elicx]

per opennextjs/opennextjs-cloudflare#862 (review)

Using this PR, the CLI will treat absolute paths as absolute, instead of always treating them as relative. Relative paths should not start with a /, i.e. you should use ./config-path.ts instead of /config-path.ts.

The behavior of the CLI is unchanged after this PR, that is:

open-next build --config-path /relative/path

will continue to treat /relative/path as relative to the baseDir == process.cwd().

[changeset-bot]

🦋 Changeset detected

Latest commit: 859bd8f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@opennextjs/aws	Patch
app-pages-router	Patch
app-router	Patch

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

[pkg-pr-new]

Open in StackBlitz

pnpm add https://pkg.pr.new/@opennextjs/aws@972

commit: 859bd8f

[vicb]

LGTM, thanks!

[sommeeeer]

LGTM, we should probably update the docs to address this change once merged. I did test this with an absolute path:

# This did work :)
node /home/user/open-next/dist/index.js build --config-path ~/hi/open-next.config.ts

[vicb]

LGTM, we should probably update the docs to address this change once merged. I did test this with an absolute path:

# This did work :)
node /home/user/open-next/dist/index.js build --config-path ~/hi/open-next.config.ts

A couple comments:

I didn't realize earlier that aws supports --config-path.

Would the current PR be a change in behavior, i.e. build --config-path /path/to would have been considered a relative path before but now requires build --config-path ./path/to.

If that is the case this is a change in behavior. I believe we can call this a "fix" rather than a breaking change but it would be nice to add details in the changeset.

Second comment is that we can use --config-path in cloudflare too, WDYT @james-elicx ?
I don't have a strong preference on this one. On one hand this would make both the CLI consistent but on the other hand cloudflare already uses --config to point to a wrangler config.
So I'm fine either way but probably something we can discuss before merging the cloudflare PR.

[james-elicx]

LGTM, we should probably update the docs to address this change once merged. I did test this with an absolute path:

# This did work :)
node /home/user/open-next/dist/index.js build --config-path ~/hi/open-next.config.ts

A couple comments:

I didn't realize earlier that aws supports --config-path.

Would the current PR be a change in behavior, i.e. build --config-path /path/to would have been considered a relative path before but now requires build --config-path ./path/to.

If that is the case this is a change in behavior. I believe we can call this a "fix" rather than a breaking change but it would be nice to add details in the changeset.

Good point. I've updated the changeset. Alternatively, we could remove the support for passing in absolute paths in the AWS CLI and leave it until they do a future breaking change?

Second comment is that we can use --config-path in cloudflare too, WDYT @james-elicx ? I don't have a strong preference on this one. On one hand this would make both the CLI consistent but on the other hand cloudflare already uses --config to point to a wrangler config. So I'm fine either way but probably something we can discuss before merging the cloudflare PR.

That would be a breaking change because we use the config path flag for the Wrangler config as part of allowing users to pass any wrangler flags to the cli.

[vicb]

Good point. I've updated the changeset. Alternatively, we could remove the support for passing in absolute paths in the AWS CLI and leave it until they do a future breaking change?

@conico974 and others, WDYT?

That would be a breaking change because we use the config path flag for the Wrangler config as part of allowing users to pass any wrangler flags to the cli.

I think it's -c / --config as I wrote earlier though?
i.e. it would not be a breaking change but --config vs --config-path could be confusing (the first one being the wrangler config and the second one the ON config)

[james-elicx]

That would be a breaking change because we use the config path flag for the Wrangler config as part of allowing users to pass any wrangler flags to the cli.

I think it's -c / --config as I wrote earlier though? i.e. it would not be a breaking change but --config vs --config-path could be confusing (the first one being the wrangler config and the second one the ON config)

Ah yes, I see, Wrangler's is config but we also gave configPath as the flag name in cloudflare, with config and c as aliases 🤦‍♂️. So that would be a breaking change still in cloudflare.

[sommeeeer]

Would the current PR be a change in behavior, i.e. build --config-path /path/to would have been considered a relative path before but now requires build --config-path ./path/to.

Ah, good catch! I confirmed this is the case.

[vicb]

Wrangler's is config but we also gave configPath as the flag name in cloudflare, with config and c as aliases 🤦‍♂️. So that would be a breaking change still in cloudflare.

@james-elicx What about:

• rename the arg to config with a single c alias (instead of configPath)
• add a "new" arg configPath

The new behavior would be:

• error early when both flags are set
• use either config or configPath when only one of them is set
• print a deprecation warning when configPath is used
• remove the deprecated configPath in the next major release

[vicb]

@james-elicx I have updated the PR description to describe the breaking change in this PR:

open-next build --config-path /relative/to/baseDir

will not treat /relative/to/baseDir as an absolute path (you would have to use ./relative/to/baseDir)

I think this is still correct?

If it is, we probably do not want that breaking outside of a major aws release

[james-elicx]

Alternatively, we could remove the support for passing in absolute paths in the AWS CLI and leave it until they do a future breaking change?

Yes, that's why I asked if we should remove that part of the change until AWS does a major release. I have no problem removing that if people prefer not to do it as part of this

[vicb]

Yes, that's why I asked if we should remove that part of the change until AWS does a major release. I have no problem removing that if people prefer not to do it as part of this

We should not introduce a breaking change in aws, confirmed offline with @conico974.

For Cloudflare we have to support both absolute (/...) and relative (./...) paths - that how wrangler --c(onfig) works and we don't want a different behavior for the Open Next config flag - that is consistency with the Cloudflare CLI is more important than consistency across aws and clouflare.

Ideally this PR can be updated to allow cf CLI support for both absolute and relative paths while not changing the behavior of aws CLI (always relative).

[james-elicx]

Thanks for confirming. I've removed that part of the change.

[vicb]

I think we usually treat internal API changes as patch changes.

I'm fine with either but maybe @conico974 has a stronger opinion?

[vicb]

@james-elicx I have added a proposal fixup to this PR, might help speed up merging this - or not :)

The idea is that:

• The CLI behavior is not changed in open-next build --config-path /relative/path, /relative/path is still relative to the baseDir
• The internal compileOpenNextConfig() takes openNextConfigPath instead of openNextConfigPath and baseDir. openNextConfigPath is actually more akin to what's we usually call a path, that is absolute when starting with a /.

This change is a patch/refactor because only an internal API is not backward compatible.

Minor changes also in this proposal:

• more JS doc
• make compileOpenNextConfigEdge return the path to the config file to be consistent with the Node conterpart.

What do you think about the proposal?

[conico974]

Sorry guys i took so long to respond here. I was convinced I did 😄
LGTM, I definitely didn't want to introduce a breaking change here

[james-elicx]

@james-elicx I have added a proposal fixup to this PR, might help speed up merging this - or not :)

The idea is that:

• The CLI behavior is not changed in open-next build --config-path /relative/path, /relative/path is still relative to the baseDir
• The internal compileOpenNextConfig() takes openNextConfigPath instead of openNextConfigPath and baseDir. openNextConfigPath is actually more akin to what's we usually call a path, that is absolute when starting with a /.

This change is a patch/refactor because only an internal API is not backward compatible.

Minor changes also in this proposal:

• more JS doc
• make compileOpenNextConfigEdge return the path to the config file to be consistent with the Node conterpart.

What do you think about the proposal?

Looks like they were just nitpicks about variable names and jsdoc, so looks good to me.

[vicb]

Looks like they were just nitpicks about variable names and jsdoc, so looks good to me.

Right!

Thanks for your work on this PR!