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

Suggestion: Cleaner output from --exclude-missing #10

Closed
jonaskello opened this issue Feb 22, 2018 · 31 comments
Closed

Suggestion: Cleaner output from --exclude-missing #10

jonaskello opened this issue Feb 22, 2018 · 31 comments
Labels

Comments

@jonaskello
Copy link

jonaskello commented Feb 22, 2018

I'm using the ability to run the same command in multiple packages, but some packages does not have the command. I can solve this by the option --exclude-missing which is great, but when you have many packages that do not have the command, the output get cluttered with @foo/bar has no bundle script, skipping missing messages. IMO if you explicitly say --exclude-missing then you don't need to see these warnings every time and it would be cleaner to just skip them.

This is what I get now:

$ yarn bundle
yarn run v1.4.1-20180207.1634
$ wsrun bundle --exclude-missing --collect-output
@foo/edit-client has no  bundle script, skipping missing
@foo/edit-plugin-core has no  bundle script, skipping missing
@foo/edit-server has no  bundle script, skipping missing
@fo/edit-client-emulator has no  bundle script, skipping missing
@fo/edit-plugin-example
$ webpack && cp packages/edit-plugin-example/bundle/index.js ../edit-client-emulator/assets/js/plugin-modules/example-plugin/
@promaster/edit-plugin-adapter
$ webpack && cp packages/edit-plugin-adapter/bundle/index.js ../edit-client-emulator/assets/js/edit-plugin-adapter/
✨  Done in 3.61s.

This is what I would like to see:

$ yarn bundle
yarn run v1.4.1-20180207.1634
$ wsrun bundle --exclude-missing --collect-output
@fo/edit-plugin-example
$ webpack && cp packages/edit-plugin-example/bundle/index.js ../edit-client-emulator/assets/js/plugin-modules/example-plugin/
@promaster/edit-plugin-adapter
$ webpack && cp packages/edit-plugin-adapter/bundle/index.js ../edit-client-emulator/assets/js/edit-plugin-adapter/
✨  Done in 3.61s.

Also, IMO it would be more natural to have --exclude-missing on by default, and maybe have something like --warn-missing or --error-missing to opt in for warnings or error when you want to make sure all packages have the command. But I guess this is a matter of taste, cleaning the output like above would be good enough for me and hopefully something most can agree on.

@spion spion added the wontfix label Feb 22, 2018
@spion
Copy link
Collaborator

spion commented Feb 22, 2018

Probably not going to do this since it will definitely be error prone and can be easily achieved by adding a grep/filter script (or by explicitly listing all the packages)

Wildcards and/or regex for package names might be a better option

@spion spion closed this as completed Feb 22, 2018
@jonaskello
Copy link
Author

I actually fail to see the motivation behind the warnings. IMO most common usage scenarios does not have scripts for all packages (start, build, bundle etc.). So not having the warnings still definitely makes sense to me, and so does making ignoring without warnings the default. I actually thought that was the way it would work. It is also the way it works in lerna run:

Run an npm script in each package that contains that script.

Given that, the --exclude-missing flag seems really obscure to me. Perhaps you could provide a scenario where ignoring non-existent scripts is dangerous/error prune or provide some motivation why the warnings were added in the first place?

@jonaskello
Copy link
Author

jonaskello commented Feb 22, 2018

Just to be clear, my main suggestion was to keep the --exclude-missing flag as-is and just not print the warnings.

If user explicitly opts out of running missing scripts by manually specifying --exclude-missing, what is the motivation behind warning him about missing scripts?

@spion
Copy link
Collaborator

spion commented Feb 22, 2018

You should list packages explicitly in this case. If you add a package you would have no indication that you may've forgotten to add a script for it if there is no warning.

Exclude missing reports for every package; what you do with that is up to you.

Lerna run's default is bad and should not be used as a reference. I'm sure yarn RFCs are going to be discussing this yarnpkg/rfcs#87

@jonaskello
Copy link
Author

You should list packages explicitly in this case

Sure, but that is quite cumbersome and error prune :-). You will have to add the script in each package and then also add each package to the main script. This is a lot of work just to get clean output. And the main script will be cluttered with a lot of package names.

If you add a package you would have no indication that you may've forgotten to add a script for it if there is no warning.

Both lerna run and oao run-script seems to be doing fine by just skipping missing scripts. And I still cannot think of a real-world scenario where this warning would be useful. Is there some common script that is important to have in all packages that is easy to forget to add? I think start, build, bundle, and test scripts are the most common ones. It will be quite obvious if you forget one of them, and in most cases all packages will not have them. I guess you have some other use-case since you added the warnings?

@spion
Copy link
Collaborator

spion commented Feb 22, 2018

yarn wsrun test is a real world scenario where you want the warning.

Refer to the yarn RFC above for discussion why lerna's behaviour is a bad idea. I tend to agree with all of that.

You will have to add the script in each package and then also add each package to the main script.

No. Just list each package you want to "bundle" in the workspace package.json

@jonaskello
Copy link
Author

jonaskello commented Feb 22, 2018

Forgetting to add a test script seems quite unlikely to me. If you take the time to make tests you are quite likely to run them too and you will notice if they don't run. At least I have never heard of someone writing tests without running them :-). IMO, a more common scenario is that all packages in a monorepo does not have or requires tests than that all packages must be forced to have tests. I'm sure both scenarios exists, I just think one is more common than the other.

Yes, I'll move to the yarn discussion. It feels more important that yarn gets it right than wsrun and I'm glad to see that the RFC is being questioned by others in this regard. Hopefully it will change to lerna/oao behaviour by default.

@spion
Copy link
Collaborator

spion commented Feb 22, 2018

It seems quite unlikely to me that you'll forget to add the package name to packages that need bundling.

Exclude missing (without warning) is a terrible default to have in an active monorepo with many contributors, where tool feedback is the quickest way to learn new things.

yarn wsrun test --exclude-missing for example will warn you if you didn't define a test script for the new repo. For bundling you have many options, one of them being to define a no-op bundling script when adding a new repo, another being to explicitly list packages that need bundling or e.g. have a convention that they'll all be named app and then have wsrun support wildcards e.g. yarn wsrun bundle 'app-*' (which I think is the right way to go)

edit: really, wildcards and conventions are the best way to deal with this:

yarn wsrun bundle 'app-*' 'plugin-*' --exclude-missing

You add a new plugin (or app), you get a warning it has no bundle script. You add modules of a different kind, you don't, since they don't need to be bundled.

@jonaskello
Copy link
Author

jonaskello commented Feb 23, 2018

It seems quite unlikely to me that you'll forget to add the package name to packages that need bundling.

My concern is not forgetting, it is making the 80% use-case cumbersome to make the 20% use-case easy. To me that is bad design.

Exclude missing (without warning) is a terrible default to have in an active monorepo with many contributors, where tool feedback is the quickest way to learn new things.

I understand the theoretical problem but I just have not seen it in the real world, neither in my projects nor anywhere else. Babel is a real-world monorepo with many contributors and seem to be doing fine with lerna. So does every other monorepo using lerna and there are plenty. If there are any real world examples of the contrary I may change my mind. Other than that my view is that the best design is to make the common use-case easy and the odd ones can require extra work to set up.

@jonaskello
Copy link
Author

jonaskello commented Feb 23, 2018

Sorry for making this issue about something that it was not originally (default behaviour). I think it is better to keep the general discussion about what is default etc. at the yarn RFC as you suggested, and keep this issue about the warning with wsrun --exclude-missing.

So about the warning, if I had a use-case where I wanted to force all scripts then I would like to have a hard error instead. Warnings are easily ignored. My reasoning is that there are two use-cases:

  • You consider it important that all packages have a script. Then you want it to break if they don't. An example would be a CI pipeline with yarn wsrun test. This already works by default in wsrun.

  • You don't consider it important that all packages have a script (--exclude-missing). In this case you don't want to be bothered by warnings. Examples would be yarn wsrun start, yarn wsrun build, yarn wsrun bundle, yarn wsrun code-gen.

So just removing the warnings from --exlude-missing would be enough for me. If I specify some switch in tsc that allows something, it will not warn me about every case where it is allowed. I can see the theoretcial use-case for an error if a script is missing. But is there a use case for the warning in --exclude-missing?

@jonaskello
Copy link
Author

Btw I tried to work around it with wsrun start '*' --exclude-missing similar to your suggestions above, but it does not work. I get the following error:

$ yarn start
yarn run v1.4.1-20180207.1634
$ wsrun start '*' --exclude-missing
Aborting execution due to previous error
error An unexpected error occurred: "Command failed.\nExit code: 1\nCommand: sh\nArguments: -c wsrun start '*' --exclude-missing\nDirectory: /XXX\nOutput:\n".
info If you think this is a bug, please open a bug report with the information provided in "/XXX/yarn-error.log".
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

@spion
Copy link
Collaborator

spion commented Feb 23, 2018

You don't consider it important that all packages have a script (--exclude-missing). In this case you don't want to be bothered by warnings. Examples would be yarn wsrun start, yarn wsrun build, yarn wsrun bundle, yarn wsrun code-gen.

Can you clarify what you mean by "you don't consider it important that all packages have a script"? Would it be a problem if, for example, none of the packages had it? Would it be a problem if a newly added package is supposed to have it, but doesn't?

@jonaskello
Copy link
Author

jonaskello commented Feb 23, 2018

Can you clarify what you mean by "you don't consider it important that all packages have a script"?

Yes, I don't consider it important that all packages have a "start" script for example, no matter how many new packages I add, only a few will have "start". So for this scenario I will specify --exclude-missing for that wsrun command, and I don't want warnings because by passing --exclude-missing I made an explicit decision to override checking all packages.

Consider passing --allowUnreachableCode to tsc. Do you then want warnings about all unreachable code?

Would it be a problem if a newly added package is supposed to have it, but doesn't?

Yes, but that is the other scenario. Then I want it to break with an error so I would not add --exlude-missing to that wsrun command.

So the question is if there is a third scenario where you want to make sure a newly added package has a script, but you don't want it to break with an error?

The point I'm trying to make is that if there is no scenario where the warnings are useful (instead of an error), then why have the warnings at all (given that they bloat the output in many common scenarios)?

@spion
Copy link
Collaborator

spion commented Feb 23, 2018

What if a user adds a package that is supposed to have a "start" script but doesn't? How will they know?

@jonaskello
Copy link
Author

jonaskello commented Feb 23, 2018

If you are trying to enforce a policy about start scripts, would you like it to give ignorable warnings that are hidden in bloated output or break with an error?

Consider a monorepo with 15 packages, where 2 have start scripts. Then wsrun start --exlcude-missing will give 13 warnings that are OK on every start. Everyone is used to seeing these warnings when starting because they are told they are OK. Now a user adds a 16th package that should have a start script. He will now get 14 warnings instead of the usual 13. Do you think he will notice it?

IMO, to make sure the that user that adds the package will know, it has to break with an error.

Yes, it would be possible to use patterns or listing packages etc. However let's consider that the warnings have no use-case that cannot be better solved with an error. The question would then be why force the users of wsrun to do a lot of complicated setup just to keep a feature that has no use-case? In order to motivate keeping the warnings I think there needs to be a case where a warning is better than an error.

@spion
Copy link
Collaborator

spion commented Feb 25, 2018

That is why --exlcude-missing has a warning. It approximates lerna behaviour, yet continues to remind that it might not be the best idea to rely on it. (Ideally the option would not exist)

Would a covention based on package name globs/patterns (e.g. all packages that end in -app must have a bundle script) work for you instead? This would not be hardcoded but instead you could do something like:

yarn run bundle '*-app' '*-plugin'

@spion
Copy link
Collaborator

spion commented Feb 25, 2018

Here is an example scenario to illustrate why I think having this option is a bad idea and should at minimum always generate a warning.

Lets say a package forgets to define a clean script to remove old built code.

  1. Package forgets a clean script, everything is fine for a while.
  2. A recent refactor in that package changed a file named "path/file.ts" to a file "path/file/index.ts" and the developer manually cleaned the build artefacts, yet forgot to add a clean script (they have the habit of running git clean at the workspace level so they didn't actually notice any issue)
  3. Both files can be used via the same statement import './path/test' but path/test.js will have precedence over path/test/index.js
  4. Another developer checks out the repo, has old build artefacts for that package
  5. They modify the source but the compiled code never reflects the new behaviour
  6. They spend hours figuring out that the old build artefact has precedence.

(Sorry if it seems contrived, but this has actually happened once)

@spion
Copy link
Collaborator

spion commented Feb 25, 2018

The general philosophy here is that wsrun would do things for compatibility sake, but if we believe something is a bad idea and there hasn't been sufficient evidence to convince us otherwise, warnings that cannot be supressed will be generated for those cases.

I know this is a bit opinionated, but I believe its the right balance.

@jonaskello
Copy link
Author

Here is an example scenario to illustrate why I think having this option is a bad idea.......

Could you clarify why a warning would be better than an error in the above scenario?

@jonaskello
Copy link
Author

jonaskello commented Feb 25, 2018

The general philosophy here is that wsrun would do things for compatibility sake, but if we believe something is a bad idea and there hasn't been sufficient evidence to convince us otherwise, warnings that cannot be supressed will be generated for those cases.

I would agree that there should be evidence to support behaviour. I would argue that the popular usage of lerna in many monorepos, combined with the lack of issues asking for warnings in lerna run, is empirical evidence that the warnings are not needed/wanted by the majority of users. Of course, If there is emprical evidence of the opposite I could be conviced that they are needed/wanted.

@spion
Copy link
Collaborator

spion commented Feb 25, 2018

Could you clarify why a warning would be better than an error in the above scenario?

An error doesn't allow you to support compatible behaviour - a warning does.

A warning is less opinionated - the idea is that we support the feature for compatibility sake but we consider its continued use to be an error.

I would agree that there should be evidence to support behaviour. I would argue that the popular usage of lerna in many monorepos, combined with the lack of issues asking for warnings in lerna run, is empirical evidence that the warnings are not needed/wanted by the majority of users. Of course, If there is emprical evidence of the opposite I could be conviced that they are needed/wanted.

Empirical evidence was provided via above "clean" example. We've had this problem, and we're determined not to have it again.

@spion
Copy link
Collaborator

spion commented Feb 25, 2018

I would argue that the popular usage of lerna in many monorepos, combined with the lack of issues asking for warnings in lerna run, is empirical evidence that the warnings are not needed/wanted by the majority of users.

In which case, may I ask - why are you using wsrun in the first place? Clearly any deviation from lerna's behaviour is a problem for you, so why not simply use lerna? After all its being used by many people and thats clear evidence they can't do no wrong.

I believe lack of issues asking for warnings isn't evidence that the issue doesn't exist (that would be argumentum ad populum). For example, people may attribute the above problem with clean on TypeScript, even though stronger conventions on Lerna's part would also eliminate the problem.

@jonaskello
Copy link
Author

An error doesn't allow you to support compatible behaviour - a warning does.

This does not really answer the question. Why in the described scenario would a warning be better than an error? Or why would you like compatibility in this scenario?

@jonaskello
Copy link
Author

In which case, may I ask - why are you using wsrun in the first place

I started using wsrun becuase I had a monorepo where nothing needs to be published. Lerna needs additional setup for yarn workspaces and comes with a lot of stuff I don't need in that kind of repos. I like wsrun except the unnecessary warnings that bloat the output in simple scenarios like yarn start.

FWIW, these discussions seem to go nowhere. I feel I have done my best to provide feedback and evidence to support the case for not bloating the output with warnings so I have nothing more to add. I'll probably be switching my non-publish repos to lerna instead and hope yarn will add intrinsic support for running scripts in monorepos without bloating the output with warnings.

@spion
Copy link
Collaborator

spion commented Feb 25, 2018

This does not really answer the question. Why in the described scenario would a warning be better than an error? Or why would you like compatibility in this scenario?

  1. An error means you can't use wsrun as a direct replacement for (a tiny subset of) lerna. We don't really want that.
  2. A warning lets you migrate gradually, but still indicates that you should eventually stop relying on that behaviour. I don't really want our new scripts to accidentally rely on that behaviour either, because we've been burned by it in the past

FWIW the plan is to support wildcard patterns to run command on package subsets, which I think is better than just ignoring missing scripts in the long run.

@jonaskello
Copy link
Author

jonaskello commented Feb 25, 2018

An error means you can't use wsrun as a direct replacement for (a tiny subset of) lerna. We don't really want that.

That is just general statements and does not specifically address the presented scenario. In fact no scenario where a warning is more helpful than an error has been presented. As for compatibility, just don't have warnings and then you have compatibility.

@spion
Copy link
Collaborator

spion commented Feb 25, 2018

Ok I'll try to be clearer than that:

In the described scenario, a warning would be better than an error if you're transitioning from lerna to wsrun. Assuming we support wildcards or patterns on package names, you would need to rename your packages, update your build scripts to include package names everywhere, update your naming conventions and switch everyone else to doing that. Thats potentially a lot of work, so the warning lets you postpone that but still use wsrun in the meanwhile and in that sense its better.

Not having the warning is worse because we (at least in my company) don't want new projects to (accidentally) rely on that behaviour without getting any feedback whatsoever since we consider it error-prone.

Its a compromise, and naturally not perfect. But its the best we can do.

@jonaskello
Copy link
Author

jonaskello commented Feb 25, 2018

Thanks, that makes it more clear. I guess the helpfulness of the warnings depends on your goal. My goal is to have compatibility without ever needing to do all of the aforementioned work. While I see now your goal is to force all users to do that sooner or later.

If wildcards were enabled, I guess wsrun '*' --exclude-missing would emulate full lerna compability? Maybe that could then be a path forward.

@jonaskello
Copy link
Author

jonaskello commented Feb 27, 2018

So I went back to lerna and I can report more things that wsrun does better:

  • Preserving output formatting. lerna run run tend to remove formatting from the tools that it runs.
  • Prefixing output with labels. I haven't found this option in lerna.
  • Path substitution, this is very helpful in vscode and lerna does not do this.

Regarding conventions for package names, I guess in your case it is ideal to force package naming convention to match what scripts to run (since you seem adamant that this is the way it should be done). However I don't think this is generally true for a wider user base. Don't get me wrong, I like naming conventions and I think most developers do. However, by forcing package naming conventions to match which scripts to run, wsrun is putting a constraint on which conventions you can use. I don't think every convention will match up with which scripts to run. I know for sure this is the case for my projects. I would rather have the freedom to come up with my own conventions for my own use-case than have a tool that runs scripts put constraints on which conventions I can use.

Btw, there is another convention at work here and that is naming of the scripts. When you add a package, you can add a script with a certain name and then it will participate in a stage of your main script. For example, if you add a new package, and add a start script to it, then it will by convention be included in start process. Similar for test and build etc. The script names in themselves are a convention.

If wsrun '*' --exclude-missing would allow us to follow only script naming convention, without forcing package naming convention, I would be willing to work on a wildcard matching PR for wsrun in order to get the above benefits over lerna run. I could of course make a PR for lerna to add the above but it seems like more work, and the code base is much larger for lerna since it does much more.

@spion
Copy link
Collaborator

spion commented Feb 27, 2018

So, (assuming nothing else changes) any feature that will cause wsrun to (given the same command)

  • run the script if it exists
  • show zero warnings or errors if the script doesn't exist

will not be added.

If you feel like the warnings are a serious problem, you're welcome to fork wsrun or use an alternative - its a simple tool designed to be easily replaceable.

@jonaskello
Copy link
Author

Understood. I still don't see why though since no real world case been presented to support the use of warnings other than to force constraints on package naming conventions, which in turn has no explanation of why it should dogmatically be the best solution for every plausible use-case of wsrun.

Given that, I guess wsrun is mostly for your internal use-cases instead of the wider open source community and that is of course your decision and I respect that. I've already thought of forking it but in my mind open source is about collaborating so many people can use the same code, rather than forking. That is why I am so persistent about finding a solution that can handle the use-cases of a wider audience. However forking is of course nice to have as a last resort when agreements cannot be reached, or collaboration is not wanted, so I guess I still might take you up on that and fork it :-).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants