refactor: join join arguments

[vicb]

fixes #142

[changeset-bot]

⚠️ No Changeset found

Latest commit: 543bda6

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

[pkg-pr-new]

Open in Stackblitz

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

commit: 543bda6

[dario-piotrowicz]

I don't understand the refactoring here... what's the issue you're trying to address? consistency? in that case shouldn't we always use either path.join or /s? I don't understand what benefit there is in having both join and /s 😕 (it feels to me like you are consistently making the code inconsistent here 🤔)

[vicb]

I don't understand the refactoring here... what's the issue you're trying to address? consistency? in that case shouldn't we always use either path.join or /s? I don't understand what benefit there is in having both join and /s 😕 (it feels to me like you are consistently making the code inconsistent here 🤔)

TL;DR: Pete noted that join normalizes the paths and using "sub/path/to" is nicer than "sub", "path", "to".

You can follow the link in link in the PR description and you'll end up the discussion

[vicb]

I'm tackling this now because I'm working on simplyfying the 15 different paths we have in the config, and dropping the .open-next/.next folder which is a duplicate of what's in .open-next/server-bundle/...

[dario-piotrowicz]

I don't understand the refactoring here... what's the issue you're trying to address? consistency? in that case shouldn't we always use either path.join or /s? I don't understand what benefit there is in having both join and /s 😕 (it feels to me like you are consistently making the code inconsistent here 🤔)

TL;DR: Pete noted that join normalizes the paths and using "sub/path/to" is nicer than "sub", "path", "to".

You can follow the link in link in the PR description and you'll end up the discussion

if that's the case then I think using path.normalize with /s instead would be the most explicit, clear and consistent solution (i.e. if you're using path.join to normalize paths, why not just use path.normalize and make the intent explicit?)

PS: I am not blocking the PR or anything, feel free to go forward with your preferred solution it's not a huge deal either way

[vicb]

If you're suggesting to use path.normalize(path.join(...)), I don't really see the point.

I think the two reasonable choice are:

• the current way path.join(base, "sub", "dir") or
• path.join(base, "sub/dir")

I'll let @petebacondarwin decide - I don't mind either way.

[dario-piotrowicz]

If you're suggesting to use path.normalize(path.join(...)), I don't really see the point.

No, I wasn't suggesting that, I was suggesting

path.normalize(`${base}/sub/dir`);

so that:

• the normalization is clear and explicit
• we consistently use /s for readability

[vicb]

Ok, let's do it as a follow up, merging this to unblock me now