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

Update lerna #10733

Merged
merged 4 commits into from Nov 21, 2019
Merged

Update lerna #10733

merged 4 commits into from Nov 21, 2019

Conversation

nicolo-ribaudo
Copy link
Member

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

package.json Outdated
@@ -73,7 +73,8 @@
"webpack-stream": "^4.0.0"
},
"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
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
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
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
Member Author

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
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);
  })

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
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.

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...

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
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.

None yet

5 participants