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

[ENHANCEMENT] Use packager commands in CONTRIBUTING.md and README.md files in app and addon blueprints #9514

Merged
merged 5 commits into from Oct 24, 2023

Conversation

elwayman02
Copy link
Contributor

  • Updates all commands to match the scripts written to package.json by the blueprint
  • Aligns the format for interpolating yarn vs npm to reduce duplication and simplify future updates

@elwayman02 elwayman02 changed the title Use yarn/npm commands for CONTRIBUTING.md Use yarn/npm commands for CONTRIBUTING.md & README.md Apr 29, 2021
@elwayman02 elwayman02 force-pushed the addon-contributing branch 2 times, most recently from 8ab9070 to 6a92252 Compare April 29, 2021 22:57
@bertdeblock
Copy link
Contributor

bertdeblock commented Apr 30, 2021

Does this issue still exist when using yarn start? If so, I'm not sure if we should use it instead of ember s?

@elwayman02
Copy link
Contributor Author

elwayman02 commented Apr 30, 2021

Seems like the bug was fixed in Yarn 2, waiting for a backport to Yarn 1. Do we make an explicit assumption or recommendation about which major version of Yarn people will use in their Ember projects?

@elwayman02
Copy link
Contributor Author

Windows tests seem to be inexplicably failing after I rebased today. Are they flaky?

@rwjblue
Copy link
Member

rwjblue commented May 17, 2021

Seems like the bug was fixed in Yarn 2, waiting for a backport to Yarn 1. Do we make an explicit assumption or recommendation about which major version of Yarn people will use in their Ember projects?

We do not currently support yarn@2, and AFAIK yarn@1 is completely EOL'ed (no bugfixes being backported).

@elwayman02
Copy link
Contributor Author

Seems like the bug was fixed in Yarn 2, waiting for a backport to Yarn 1. Do we make an explicit assumption or recommendation about which major version of Yarn people will use in their Ember projects?

We do not currently support yarn@2, and AFAIK yarn@1 is completely EOL'ed (no bugfixes being backported).

Is there an RFC to drop yarn support in Ember? It seems we should either support it (and document it) or not, but half support is generally not a great experience.

To give a bit more context on this PR: Part of the reason I created it is that telling people to run ember test instead of yarn test can have entirely different outcomes for many projects. What if the author of an addon purposefully sets up yarn test to run with ember-exam, for example, or any number of other script modifications? I don't think that they should need to manually change the default documentation for a script that already existed, when all they did was modify the internals of the script.

Putting the known yarn bug aside, this is still true for npm test as well. The default documentation should prompt people to use npm test and npm start commands so they don't need to learn new commands for every project they work on. The whole point of these standardized scripts is to allow maintainers to customize the scripts themselves while having a standard command that everyone can run in any project and get an expected output regardless of what's going on under the hood.

If our generated documentation says to run ember test and ember serve instead of npm test, yarn test, npm start, and yarn start, then we're undermining that industry standard and confusing people by making it seem that they need to know these special ember commands instead of interfacing with the project the way they would any other node project.

So, while I'm sympathetic to the fact that there is a bug in yarn, I would argue that it's a bug in yarn, not a bug on our side. Furthermore, it's a minor UX bug that doesn't cause anything to break. I'm open to the idea that we should drop support for Yarn (though that should be discussed in an RFC rather than here, of course), but I think if we're going to support a --yarn flag that allows people to purposefully opt-in to using yarn, then we should generate correct documentation as well.

@elwayman02
Copy link
Contributor Author

@rwjblue How would you like to move forward?

@rwjblue
Copy link
Member

rwjblue commented May 27, 2021

I actually think these changes are good and I've preferred using yarn/npm vs requiring everyone have a global ember binary for a while, but ultimately this changes how people are expected to interact with the project in a lot of ways. I think we need an RFC.

@elwayman02
Copy link
Contributor Author

Isn't this PR just documenting scripts that have already been in place in Ember projects for years, though? I'd argue that this choice was already made a long time ago, and we just didn't document it fully. What practical decision do you think still needs to be discussed at this point?

@elwayman02
Copy link
Contributor Author

elwayman02 commented May 28, 2021

Putting it another way - do we expect that a realistic possible outcome is that we're going to remove these scripts from package.json? If not, what is the purpose of the RFC? Just discussing whether we should document the default scripts we've been shipping for years?

@rwjblue
Copy link
Member

rwjblue commented Jun 11, 2021

What practical decision do you think still needs to be discussed at this point?

The point I'm making is that this PR is effectively saying that the default way we expect people to interact with their app is via yarn scripts, not the ember global. That definitely is RFC worthy.

@elwayman02
Copy link
Contributor Author

I would argue that it has been the de facto default for years, but I can see there is disagreement on that. Unfortunately, such an RFC is something I have neither the time nor motivation to write up, so unless someone else is going to volunteer to drive that discussion, this PR appears to be at an impasse.

@bertdeblock
Copy link
Contributor

bertdeblock commented Jun 16, 2021

An issue in the RFCs repo might be a good start?

@elwayman02
Copy link
Contributor Author

An issue in the RFCs repo might be a good start?

Go for it.

@elwayman02
Copy link
Contributor Author

elwayman02 commented Oct 9, 2021

FWIW, the known yarn bug doesn't even exist on Windows; exiting a yarn script with ctrl-c works perfectly in my experience. So, we're basically holding off on documenting the primary way that most people interact with Ember's default scripts because of a bug that only occurs in one of the possible package managers (that is EOL'd and isn't even the default) on some operating systems/terminal platforms. This really doesn't seem like a good tradeoff to me.

@bertdeblock
Copy link
Contributor

@elwayman02 Are you still interested in pursuing this PR? Should we close if not?

@elwayman02
Copy link
Contributor Author

elwayman02 commented Jun 16, 2022 via email

@bertdeblock
Copy link
Contributor

There's more to it than just this PR though. Using the ember global is also what's used in the guides etc.

@elwayman02
Copy link
Contributor Author

elwayman02 commented Jun 16, 2022 via email

@bertdeblock
Copy link
Contributor

I agree, but why not coordinate this via an RFC (issue) as suggested in this thread?
I think the learning team would also like to know about / weigh in on this.

@elwayman02
Copy link
Contributor Author

elwayman02 commented Jun 16, 2022 via email

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Jun 16, 2022

this whole thread feels like a "too many cooks" problem 😅

an RFC for this sort of change isn't worth the time. we can respectfully discuss learning / tradeoffs in this thread (and I think we already have)

NullVoxPopuli
NullVoxPopuli previously approved these changes Jun 16, 2022
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli left a 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.

in practice, folks will use the script-runner capabilities of their package-manager more than ember.

(except for maybe ember s).

These are good changes

* `ember test --server` – Runs the test suite in "watch mode"
* `ember try:each` – Runs the test suite against multiple Ember versions
* `<% if (yarn) { %>yarn<% } else { %>npm run<% } %> test:ember` – Runs the test suite on the current Ember version
* `<% if (yarn) { %>yarn<% } else { %>npm run<% } %> test:ember --server` – Runs the test suite in "watch mode"
Copy link
Contributor

Choose a reason for hiding this comment

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

The npm version needs an extra -- for the --server flag to work.

* `ember test`
* `ember test --server`
* `<% if (yarn) { %>yarn<% } else { %>npm run<% } %> test:ember`
* `<% if (yarn) { %>yarn<% } else { %>npm run<% } %> test:ember --server`
Copy link
Contributor

Choose a reason for hiding this comment

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

The npm version needs an extra -- for the --server flag to work.

@bertdeblock
Copy link
Contributor

I've asked on Discord if the learning team would be okay with merging these changes:
https://discord.com/channels/480462759797063690/480777444203429888/987678229185835018

@jenweber
Copy link
Contributor

Hello! Thanks for raising this. I consider this PR to be currently blocked, unless @rwjblue says otherwise. The core teams have a consensus-based decision making model, which we should lean on here to guide the next step.

An RFC would be beneficial, even if it was not required in the end. I can try to find a volunteer to write it, and therefore unblock this PR. This may feel like a small change but it does ripple through guides and API docs.

@NullVoxPopuli
Copy link
Contributor

An RFC would be beneficial

To preface, I say this with all the love of Ember in the world....

This is a giant problem with this community. We cannot be doing RFCs for every little basic-AF change.

The fact that this RFC has already been open for a year and had more discussion than an RFC would have is already a crime against humanity.

The resistance to basic contributions here is super alienating towards contributors, and is likely a big part of why our contributor count is so low.

@jenweber
Copy link
Contributor

Good news, there are 3 volunteers to help with an RFC. Progress will be tracked in emberjs/rfcs#827 and I've set a goal of 2 weeks to have a rough draft.

We have some rules of thumb about when something may need an RFC. They are outlined in the RFCs readme. The third bullet point seems relevant. I understand the frustrations with the RFC process and do what I can to make it easier/faster to participate.

@jenweber
Copy link
Contributor

Just an update - there's a draft of the RFC and it will be PR'd soon by the authors.

@bertdeblock
Copy link
Contributor

@jenweber Would you be okay with already getting these changes in even though the RFC is still open? It seems everyone agrees we should do this?

@jenweber
Copy link
Contributor

jenweber commented Oct 2, 2022

Good question. I don’t think it’s my call to make though. Let me follow up with the RFC authors and see if we can get those edits in ASAP

@bertdeblock bertdeblock self-assigned this Oct 5, 2022
@bertdeblock bertdeblock changed the title Use yarn/npm commands for CONTRIBUTING.md & README.md [ENHANCEMENT] Use packager commands for CONTRIBUTING.md and README.md files in app and addon blueprints Oct 5, 2022
@bertdeblock bertdeblock changed the title [ENHANCEMENT] Use packager commands for CONTRIBUTING.md and README.md files in app and addon blueprints [ENHANCEMENT] Use packager commands in CONTRIBUTING.md and README.md files in app and addon blueprints Oct 5, 2022
@NullVoxPopuli
Copy link
Contributor

@elwayman02 any chance you have time to rebase? looks like tests are failing 🙈

* Updates all commands to match the scripts written to `package.json` by the blueprint
* Aligns the format for interpolating yarn vs npm to reduce duplication and simplify future updates
@elwayman02
Copy link
Contributor Author

elwayman02 commented Oct 3, 2023

@NullVoxPopuli I rebased, but FYI there will probably need to be a follow-up in some files to account for pnpm now, where previously the output was the same in all cases because it was doing ember start or whatever. I don't have the bandwidth to make further changes or debug testing issues, but hopefully this helps. <3

@NullVoxPopuli NullVoxPopuli merged commit 7338655 into ember-cli:master Oct 24, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants