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

Breaking change in unstable workspaces configuration field #24456

Closed
nberlette opened this issue Jul 6, 2024 · 4 comments · Fixed by #24483
Closed

Breaking change in unstable workspaces configuration field #24456

nberlette opened this issue Jul 6, 2024 · 4 comments · Fixed by #24483

Comments

@nberlette
Copy link
Contributor

Forgive me if this has been discussed or announced somewhere and if I've overlooked it. I couldn't find any issues related to this after a cursory search just now. Also, please forgive me in advance for this somewhat longwinded post.

Background:
I've been using the new-ish workspaces feature over the last few months in my monorepo projects that I manage with Deno. The root deno.json files included a "workspaces" field that instructed Deno of the child projects living beneath it. This setup has been working just fine for at least a month or two now.

Issue:
Fast forward to Wednesday or Thursday last week, I spooled up a codespace configured to upgrade my Deno installation to the latest canary build upon start-up. As soon as I attempted to run a task to execute all unit tests in the monorepo and generate coverage for all workspace members, I encountered this error:

The 'workspaces' field was ignored. Maybe you mean 'workspace'?

No, I definitely did not. At this point, I felt like I was experiencing a Bernstein/Berenstain Bears situation. I checked my configuration files for several other projects on my local machine, confirming that on a slightly older canary build, the workspaces configuration worked fine.

Concerns:

  1. Intuitiveness: The previous name "workspaces" is much more intuitive than its singular sibling "workspace". The former clearly indicates a list of projects managed by that configuration file and aligns with existing naming conventions established by package managers like pnpm, yarn, and npm.

  2. Abrupt Change: This unannounced breaking change in a patch/pre-patch release is counterintuitive and shortsighted. I understand it is a canary build, which is why I've decided to air my concerns before it is rolled out for good.

  3. User Impact: This will impact users who have been eagerly adapting their monorepos to use Deno given its newfound support for the pattern. Without a warning or transition plan, it effectively breaks every project like mine from one version to the next. I'd imagine I wouldn't be the only one upset about that.

Suggested Resolution:

  • Revert the change (the most reasonable, if you ask me)
  • Maintain backward compatibility with the "workspaces" field during the adoption period for the new naming convention.
  • Provide a clear migration guide or deprecation notice to help users transition smoothly.

I appreciate your attention to this matter and look forward to your response.

@nberlette nberlette changed the title Breaking Change in Workspace Configuration Field Breaking Change in Workspaces Configuration Field Jul 6, 2024
@dsherret
Copy link
Member

dsherret commented Jul 7, 2024

Thanks for using the new workspace feature, but it's been considered unstable and undocumented so expect breaking changes (see how the flag was removed, but it's still considered unstable #21891). I don't think it's even been announced. I'm updating the title of this issue to reflect this so it's clear. It's going to be released next week in Deno 1.45.

  • Maintain backward compatibility with the "workspaces" field during the adoption period for the new naming convention.
  • Provide a clear migration guide or deprecation notice to help users transition smoothly.

If the field does keep it's name, we won't be doing this because it's been an unstable feature.

@dsherret dsherret changed the title Breaking Change in Workspaces Configuration Field Breaking change in unstable workspaces configuration field Jul 7, 2024
@dsherret
Copy link
Member

dsherret commented Jul 7, 2024

The previous name "workspaces" is much more intuitive than its singular sibling "workspace".

I think Cargo has this right (https://doc.rust-lang.org/book/ch14-03-cargo-workspaces.html#creating-a-workspace). It's a singular workspace with multiple workspace members. The current form is essentially a shorthand of the following (though this isn't supported atm) and there might be more properties added under there in the future:

"workspace": {
  "members": ["./member-1", "./member-2"]
}

@nayeemrmn
Copy link
Collaborator

I don't think the name workspaces as used in node pm's are widely thought of as intuitive. NPM did this to align with yarn, and yarn's docs are inconsistent about it as of posting: https://classic.yarnpkg.com/lang/en/docs/workspaces/#toc-how-to-use-it

Add the following in a package.json file. Starting from now on, we’ll call this directory the “workspace root”:

Namely, the usage of 'workspace root' as opposed to 'root workspace' (FWIW npm's docs are a lot more careful). The conclusion in this discussion npm/feedback#510 is that they didn't want to diverge from yarn and don't want to make a breaking change after the fact. But you can see it's controversial. I speculate that the reverse wouldn't have been.

As well as the cargo example above, we align with VSCode's effective usage of 'workspace'.

dsherret added a commit to denoland/deno_config that referenced this issue Jul 9, 2024
This makes the naming clear as `"workspace": [...]` is shortform for
`"workspace": { "members": [...] }` similar to cargo
(https://doc.rust-lang.org/book/ch14-03-cargo-workspaces.html#creating-a-workspace)
and leaves this open for future properties.

denoland/deno#24456 (comment)
@nberlette
Copy link
Contributor Author

nberlette commented Aug 1, 2024

Hey @nayeemrmn and @dsherret, sorry for the late reply and for commenting on a closed issue. I've been AFK dealing with family stuff and birthday celebrations, so I'm just now getting back to everything.

Looking back, I realize I was a bit naive in creating this issue without fully understanding (or researching) that I was dealing with a pre-release and undocumented feature 😬. In hindsight, I retract all my previous statements.

Lately, I've been diving deep into Rust, and I have to say, following the Cargo semantics of workspace.members really is a much more intuitive approach. So, my bad for the oversight in the original post, and thanks for pointing out something that was right in front of me that I somehow missed.


Edit: I also wanted to mention that I just saw the new documentation for the workspaces feature, and it does a phenomenal job explaining it all. 👏🏼 You guys rock

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 a pull request may close this issue.

3 participants