Skip to content

Conversation

@mgol
Copy link
Member

@mgol mgol commented Mar 9, 2020

Keywords: 2FA, OTP, npm

I tried to not add any new packages and use the native readline API but the most basic example in the API docs cause the provided output to be printed twice. I was testing various workarounds but I couldn't make it work.

This increases the checked-in node_modules size from 584 KB to 932 KB. Way better than inquirer that increases it to 23 MB. 😮

@mgol mgol requested review from arschmitz, gibson042 and timmywil March 9, 2020 19:00
@timmywil
Copy link
Member

timmywil commented Mar 9, 2020

How would we feel about removing node_modules from tracking and committing a package-lock.json? I understand that precludes yarn usage, but I'd rather do that than continue to commit the node modules. Besides, yarn 2 is moving away from using the node_modules folder at all.

@mgol
Copy link
Member Author

mgol commented Mar 9, 2020

@timmywil That's a possibility. I can't speak for what exactly were Scott's idea here to skip it other than the lack of a lockfile and reducing dependency on external code during the release - but we have such a dependency anyway as the release process needs to download dependencies of the project it releases.

One caveat is package-lock.json is not stable when installation is done via a different npm version than the one used to generate the lockfile. But maybe that's a lesser evil here.

What do others think?

@mgol
Copy link
Member Author

mgol commented Mar 10, 2020

BTW, @timmywil, regardless of the decision, I feel this may be a matter to settle separately to this PR to not block its merge. What do you think?

@timmywil
Copy link
Member

I agree, but it'd be a nice-to-have before the next jQuery release.

lib/npm.js Outdated
safety = Release.isTest ? "--dry-run" : "",
npmTags = Release.npmTags();

const otp = await Release._getNpmOtp();
Copy link
Member

Choose a reason for hiding this comment

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

The mixture of const and var is a bit odd to me. Perhaps we should stick with var and then convert everything in another PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if I have resources right now to convert everything. Would converting vars in the _publishNpm method be enough here?

Copy link
Member

@timmywil timmywil Mar 10, 2020

Choose a reason for hiding this comment

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

I think we can use eslint and just run the auto fix to do the conversion.

Copy link
Member

Choose a reason for hiding this comment

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

I can do that btw. For this PR, I'd say switch consts to vars.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool.

About the rule:

The --fix option on the command line can automatically fix some of the problems reported by this rule.

I wonder how much it can actually auto-convert. An autofixer that fixes all the occurrences would most likely be impossible.

Copy link
Member Author

Choose a reason for hiding this comment

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

PR updated, all consts removed.

@mgol mgol merged commit 00c8feb into jquery:master Mar 13, 2020
@mgol mgol deleted the npm-otp branch March 13, 2020 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants