Skip to content

Update lerna#10733

Merged
nicolo-ribaudo merged 4 commits intobabel:masterfrom
nicolo-ribaudo:update-lerna
Nov 21, 2019
Merged

Update lerna#10733
nicolo-ribaudo merged 4 commits intobabel:masterfrom
nicolo-ribaudo:update-lerna

Conversation

@nicolo-ribaudo
Copy link
Copy Markdown
Member

Q                       A
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes? Yes
License MIT

Comment thread package.json Outdated
"resolutions": {
"@lerna/**/@lerna/collect-updates": "https://github.com/babel/lerna.git#babel-collect-updates"
"@lerna/**/@lerna/collect-updates": "https://github.com/babel/lerna.git#babel-collect-updates",
"@lerna/cli/yargs": "14.2.0"
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Version 14.2.1 generates an error:

TypeError: Cannot delete property '--' of #<Object>
    at Object.Yargs.self._copyDoubleDash (/home/nicolo/Documenti/dev/babel/babel/node_modules/@lerna/cli/node_modules/yargs/yargs.js:1196:5)
    at Object.parseArgs [as _parseArgs] (/home/nicolo/Documenti/dev/babel/babel/node_modules/@lerna/cli/node_modules/yargs/yargs.js:1097:60)
    at Object.parse (/home/nicolo/Documenti/dev/babel/babel/node_modules/@lerna/cli/node_modules/yargs/yargs.js:578:25)
    at main (/home/nicolo/Documenti/dev/babel/babel/node_modules/lerna/index.js:44:6)
    at Object.<anonymous> (/home/nicolo/Documenti/dev/babel/babel/node_modules/lerna/cli.js:11:15)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
    at Module.load (internal/modules/cjs/loader.js:599:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
    at Function.Module._load (internal/modules/cjs/loader.js:530:3)

because of

babel/Makefile

Line 240 in 6ba1131

yarn lerna bootstrap -- -- --ignore-engines

cc @bcoe (author of the commit in v14.2.1 - yargs/yargs@e78e76e) is this a known issue fixed in v15?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we work around it by

node $(npm bin)/lerna bootstrap -- --ignore-engines

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@nicolo-ribaudo something outside of yargs, in lerna is calling Object.seal() on the argv object, before the parse is finished (there's a two pass parse that temporarily stores positional arguments in -- and then merges them into the final parse).

Copy link
Copy Markdown
Member Author

@nicolo-ribaudo nicolo-ribaudo Nov 18, 2019

Choose a reason for hiding this comment

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

@JLHwung Nope:

node node_modules/.bin/lerna bootstrap -- --ignore-engines
/home/nicolo/Documenti/dev/babel/babel/node_modules/@lerna/cli/node_modules/yargs/yargs.js:1184
      else throw err
           ^

TypeError: Cannot delete property '--' of #<Object>
    at Object.Yargs.self._copyDoubleDash (/home/nicolo/Documenti/dev/babel/babel/node_modules/@lerna/cli/node_modules/yargs/yargs.js:1196:5)
    at Object.parseArgs [as _parseArgs] (/home/nicolo/Documenti/dev/babel/babel/node_modules/@lerna/cli/node_modules/yargs/yargs.js:1097:60)
    at Object.parse (/home/nicolo/Documenti/dev/babel/babel/node_modules/@lerna/cli/node_modules/yargs/yargs.js:578:25)
    at main (/home/nicolo/Documenti/dev/babel/babel/node_modules/lerna/index.js:44:6)
    at Object.<anonymous> (/home/nicolo/Documenti/dev/babel/babel/node_modules/lerna/cli.js:11:15)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
    at Module.load (internal/modules/cjs/loader.js:599:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
    at Function.Module._load (internal/modules/cjs/loader.js:530:3)
make: *** [Makefile:240: lerna-bootstrap] Error 1

@bcoe Thanks! I'll fix it in lerna then.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@nicolo-ribaudo could you try commenting out this line?

https://github.com/lerna/lerna/blame/2c487fe5f612739970eb08026b2bf7da82a32b66/core/command/index.js#L99

I haven't quite figured out how to make a reproduction in yargs; If we can figure out a reproduction, I'd be happy to add a try/catch, and just skip the delete step for this edge case.

This does not fail, like I would hope:

  // see: https://github.com/babel/babel/pull/10733
  it('does not fail if Object.seal() has been called on argv', () => {
    Object.seal(process.argv)
    const argv = yargs()
      .command('cmd <version>', 'a command', () => {}, (argv) => {
        Object.freeze(argv)
      }).argv
    console.info(argv);
  })

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ack, sorry folks. I don't even remember why I cared about freezing the argv. I can remove it, as it serves basically no purpose.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@evocateur I think you should be able to freeze the object once it's been passed to a command ... so it feels like yargs stepping on your toes as well.

I was thinking this morning, perhaps we pass a shallow clone of the object to the command rather than the actual argv.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm fixing it in lerna/lerna@8ca85a4 by deep cloning argv before I mutate it. Releasing momentarily...

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

lerna v3.18.5 published with the aforementioned fix

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thanks @evocateur!

evocateur added a commit to lerna/lerna that referenced this pull request Nov 20, 2019
evocateur added a commit to lerna/lerna that referenced this pull request Nov 20, 2019
The problem encountered in babel/babel#10733 has been fixed, but bump it anyway.
@nicolo-ribaudo nicolo-ribaudo merged commit cc51f2a into babel:master Nov 21, 2019
@nicolo-ribaudo nicolo-ribaudo deleted the update-lerna branch November 21, 2019 00:05
@lock lock Bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Feb 21, 2020
@lock lock Bot locked as resolved and limited conversation to collaborators Feb 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Dependency ⬆️

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants