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

Avoid duplicate workspace names. #120

Merged
merged 2 commits into from
Aug 9, 2023

Conversation

NullVoxPopuli
Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli commented May 16, 2023

Discovered while debugging: #118
If someone wants to use a newer yarn, they get this error:

WARNING: Yarn v2 is not fully supported. Proceeding with yarn: 4.0.0-rc.42

🚧  Installing packages... This might take a couple of minutes.
Command failed with exit code 1: yarn add --dev ember-cli-typescript
Internal Error: Duplicate workspace name ts-project: /Users/psego/Development/tmp/ts-project/ts-project conflicts with /Users/psego/Development/tmp/ts-project
    at St.addWorkspace (/Users/psego/.volta/tools/image/yarn/4.0.0-rc.42/bin/yarn.js:204:1949)
    at async St.setupWorkspaces (/Users/psego/.volta/tools/image/yarn/4.0.0-rc.42/bin/yarn.js:204:1735)
    at async Function.find (/Users/psego/.volta/tools/image/yarn/4.0.0-rc.42/bin/yarn.js:201:8463)
    at async P0.execute (/Users/psego/.volta/tools/image/yarn/4.0.0-rc.42/bin/yarn.js:386:14007)
    at async P0.validateAndExecute (/Users/psego/.volta/tools/image/yarn/4.0.0-rc.42/bin/yarn.js:91:12775)
    at async vo.run (/Users/psego/.volta/tools/image/yarn/4.0.0-rc.42/bin/yarn.js:95:3074)
    at async vo.runExit (/Users/psego/.volta/tools/image/yarn/4.0.0-rc.42/bin/yarn.js:95:3258)
    at async o (/Users/psego/.volta/tools/image/yarn/4.0.0-rc.42/bin/yarn.js:386:4014)
    at async r (/Users/psego/.volta/tools/image/yarn/4.0.0-rc.42/bin/yarn.js:386:2118)


Stack Trace and Error Report: /var/folders/wk/w99lck4x7_5930c7gj65r3s40000gp/T/error.dump.2e2f8c9d013a9b63fda95e62ac7d25ac.log

 ~/Development/tmp took 14s
❯  yarn --version
4.0.0-rc.42

More info here: #114
tl;dr: yarn@v1 is the only yarn that allows workspaces to share a name

It's a goofiness to have multiple workspaces with the same name anyway, and yarn isn't the only tool that will error in this situation. Since duplicating the name with the addon name provides no value, I think it makes sense to add a suffix to the root package.json#name

@simonihmig
Copy link
Collaborator

IIRC I did this because otherwise ember-cli-update would run the blueprint with the wrong name. Although currently we have also other problems with ecu: #93. Which is quite bad, as with all the update here, we'd want a way to update our adodns easily, without the manual hassle...

It's a goofiness to have multiple workspaces with the same name anyway

Is the root package.json considered a workspace?

@NullVoxPopuli
Copy link
Collaborator Author

NullVoxPopuli commented May 22, 2023

Is the root package.json considered a workspace?

in pnpm, yarn@v3, and yarn@v4 it is.

Also in pnpm, you can do this:

package.json
packages/
  package-name/
    package.json
    tests/
      package.json

Which I'm going to be exploring with these efforts: #121

ember-cli-update would run the blueprint with the wrong name

we probably need to do some series of fixes to ECU.

@ef4
Copy link
Contributor

ef4 commented May 23, 2023

I think the top-level doesn't need a name at all. The embroider monorepo doesn't have one and has been used under both yarn and pnpm.

@NullVoxPopuli
Copy link
Collaborator Author

I think the top-level doesn't need a name at all.

Testing with:

npm: ✅

rm -rf test-addon; \
  npx ember-cli@latest addon test-addon -b ../OpenSource/addon-blueprint --no-yarn \
  && ( cd test-addon && npm run lint )

yarn@v1: ✅

volta install yarn@1
yarn --version # 1.x.y
rm -rf test-addon; \
  npx ember-cli@latest addon test-addon -b ../OpenSource/addon-blueprint --yarn \
  && ( cd test-addon && yarn lint )

yarn@v3: ✅: install works, probably fine. ⚠️ : I couldn't figure out how to run a command in each workspace

volta install yarn@3
yarn --version # 3.x.y
rm -rf test-addon; \
  npx ember-cli@latest addon test-addon -b ../OpenSource/addon-blueprint --yarn \
  && ( cd test-addon && yarn lint )

yarn@v4: ✅: install works, probably fine. ⚠️ : I couldn't figure out how to run a command in each workspace

volta install yarn
yarn --version # 4.x.y-rc.z
rm -rf test-addon; \
  npx ember-cli@latest addon test-addon -b ../OpenSource/addon-blueprint --yarn \
  && ( cd test-addon && yarn lint )

pnpm@7 ✅

volta install pnpm@7
rm -rf test-addon; \
  npx ember-cli@latest addon test-addon -b ../OpenSource/addon-blueprint --pnpm --skip-npm \
  && ( cd test-addon && pnpm install; pnpm lint )

pnpm@8 ✅

volta install pnpm@8
rm -rf test-addon; \
  npx ember-cli@latest addon test-addon -b ../OpenSource/addon-blueprint --pnpm --skip-npm \
  && ( cd test-addon && pnpm install; pnpm lint )

Copy link
Collaborator

@simonihmig simonihmig left a comment

Choose a reason for hiding this comment

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

I'm ok with this chance... if we are also agreeing to drop support for ember-cli-update. Which this change will manifest, unless ecu is updated to provide other means to work with v2 projects. Again, while there is currently #93 waiting for a fix, the current change here will make it much more difficult to bring back ecu support. And that is publicly advertised as a feature of this blueprint, so this is a significant change...

@NullVoxPopuli
Copy link
Collaborator Author

NullVoxPopuli commented May 24, 2023

if we are also agreeing to drop support for ember-cli-update.

I think we need to fix ECU, tbh -- it's been without maintenance for quite some time.
There are some assumptions it makes which I think we can get rid of to try to make it more universal.
If ya'll want to block addon-blueprint PRs (really just this one?) until ECU is fixed, that's fine by me.
I don't personally use ECU, as it hasn't worked for me in the past 4+ years i've been doing Ember professionally (related to monorepos)... which... probably either means it needs lots and lots of fixing, or the git strategy it uses is flawed. idk.

I understand that other people use ECU, and rely on it though.
I don't personally have the time to fix its issues atm.
I could maybe take a look after emberconf or later. 🤷

@mansona
Copy link
Member

mansona commented May 26, 2023

We discussed this in the meeting this week, we need ECU to work 👍 and we should probably not have modern yarn error 🤷

If we need this change then we need to make ECU work like this, but if we can do a smaller fix that fixes yarn and keeps ECU working then we might want to consider doing that in the mean time

@mansona
Copy link
Member

mansona commented Aug 9, 2023

We discussed this today and we're happy to merge for now and we can make sure that ember-cli-update works as a separate effort 👍

@mansona mansona merged commit cfa47d2 into main Aug 9, 2023
@mansona mansona deleted the duplicate-workspace-names-are-not-allowed branch August 9, 2023 15:42
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.

4 participants