mysticatea / npm-run-all

A CLI tool to run multiple npm-scripts in parallel or sequential.
MIT License
5.72k stars 240 forks source link

How does run-s determine the order of a globbed pattern? #187

Open thegoatherder opened 4 years ago

thegoatherder commented 4 years ago

Given a run-scripts config as follows:

"scripts": {
    "reinstall": "run-s reinstall:* build:install",
    "reinstall:modules": "npx --ignore-existing --quiet rimraf package-lock.json node_modules",
    "reinstall:cache": "yarn cache clean",
}

And the command: npm run reinstall

How does npm-run-all determine the run sequence for run-s reinstall:*? Is it deterministic? Are run-scripts executed in the exact order they appear in package.json or are the keys sorted alphanumerically first?

nettybun commented 4 years ago

I also wanted to know this so I dug through the code (at bf91f94ce597aa61da37d2e4208ce8c48bc86673)

It comes down to readPackageJson() which, despite using a few other modules, is basically

const pkg = JSON.parse(fs.readFileSync('./package.json'))
const tasks = Object.keys(pkg.scripts)

I'm paraphrasing, but it's all on Object.keys. Now, the internet will tell you to not trust the order of object keys, and that it isn't technically predictable or guaranteed... I think it's fine. All browsers and Node.js will return the insertion order, basically.

In my experience, using run-s build:* has always been the order the package.json is written. I've run it hundreds of times and nothing broke yet 🤞🤞

https://github.com/mysticatea/npm-run-all/blob/master/lib/index.js#L257 https://github.com/mysticatea/npm-run-all/blob/master/lib/read-package-json.js#L28

matzkoh commented 2 years ago

This is an important topic for us to deliver value accurately.

You are all familiar with Object.keys and the Node.js implementation.

Currently, key-values are fetched in insertion order, and that's probably never going to change.

However, I feel that npm-run-all needs to take a stance.

@mysticatea

Is it possible to document that the order in which run-s is used with the glob pattern is guaranteed, or not guaranteed at all?

Personally, I prefer the latter. If you are concerned about the order, I think you should write it explicitly without using *.

iainmerrick commented 2 years ago

I've just started using npm-run-all and I was wondering about exactly this.

It would definitely be good to document the behavior.

I'd be in favour of strictly executing globs in the order listed in package.json, because:

kylecordes commented 2 years ago

The current behavior appears to be: whatever order they are in package.json.

I prefer that this be documented and continue as-is, for the reasons listed above.

If for some reason the choices made to continue having it undocumented, I guess randomize each time? Because then at least users like me will no longer accidentally depend on the order.

thegoatherder commented 2 years ago

I’m also in favour of documenting it as “same order as package.json” and locking this behaviour. Any change to the behaviour should be considered a “breaking change” and incur a semver major version increment.

Changing the behaviour at this stage certainly seems wrong. I’m utterly against @kylecordes suggestion randomising the order as this introduces non-deterministic behaviour (though I acknowledge it was not his first choice!).

run-s means run sequentially. Randomness should not come into play in any sequential behaviour IMO. We could add a run-r command for random, though I’m not sure there’s many use cases for that?

Are we able to put this to bed? If I update the docs to explicitly state it’s sequential based on the order of package.Json, will the maintainers accept my PR?

nettybun commented 2 years ago

Any change to the behaviour should be considered a “breaking change” and incur a semver major version increment.

This is outside of the package author's control. It depends entirely on the engine. It so happens that all engines today follow insertion order, but this isn't part of the spec. Other engines may choose to do it differently, like QuickJS or Hermes or SerenityOS' libjs. There are a lot of engines out there. If Node v19 comes out and accidentally changes the Object.keys behaviour there'd be no code changes here, and no reason to semver increment this package.

If you absolutely need to have it follow insertion order then you need to read the package.json without using JSON.parse or Object.keys. It's doable, some .indexOf("scripts:")... etc but not likely worth it.

I'm sorry the spec for ecmascript left this open for implementation specific behaviour... I don't like it either.

thegoatherder commented 2 years ago

If you absolutely need to have it follow insertion order then you need to read the package.json without using JSON.parse or Object.keys. It's doable, some .indexOf("scripts:")... etc but not likely worth it.

@heyheyhello I would argue it’s entirely worth it. This is a script runner. Deterministic behaviour should be a cornerstone pillar of running scripts sequentially. It should behave the same on all platforms/engines. There is no Use Case (at least that I can conceive of) where I don’t care about the order of execution in a sequential run. If I truly don’t care then I would always choose run-p as it gives better performance in a scenario where order is not important.

I know the counter argument is “if order is so important, then explicitly state all scripts and don’t use globs” - which is a fair point but it begs the question why have globs at all? The whole point of globs is to reduce maintenance when new entries are added. I welcome the use of globs but not at the sacrifice of determinism.

We don’t need to rely on ECMASCRIPT spec decisions to have a solid library that won’t break people’s task runners when the underlying spec changes. It’s all about taking ownership of the spec, surely?

thegoatherder commented 2 years ago

PS - we could debate this back and forth forever. Perhaps the way out is via feature request (or an offer to contribute if the maintainers will accept such a PR) we could consider changing the CLI?

Proposal:

I use this library for most projects I work on and would be happy to contribute to above.

@mysticatea - do you have any views on above please? Are you still actively maintaining this project or have you moved on?

kylecordes commented 2 years ago

To clarify, randomizing is indeed a terrible idea.

My point was just this: either document and continue to ensure the current behavior, or change to some other deterministic behavior (breaking change, major version).

If the choice is made that the order will continue to be nondeterministic, in this case the only way to give fair warning of this non-determinism is to randomize. Because that is awful but still slightly less bad than having an order that appears to be deterministic, but which might break at any time.