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

Adds compatibility for cordovaDependencies engine #3786

Merged
merged 3 commits into from
Jul 6, 2017
Merged

Adds compatibility for cordovaDependencies engine #3786

merged 3 commits into from
Jul 6, 2017

Conversation

bytenik
Copy link
Contributor

@bytenik bytenik commented Jul 1, 2017

See https://cordova.apache.org/docs/en/latest/guide/hybrid/plugins/#specifying-cordova-dependencies for a description of when the cordovaDependencies engine is used.

Without this change, freshly installed Cordova plugins generate errors on yarn add. This was mentioned in #3437 a while back but no one ever made a pull request to resolve it.

Copy link
Member

@BYK BYK left a comment

Choose a reason for hiding this comment

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

Hi, thanks for the PR!

Can you run yarn prettier and update the PR with those changes?

@BYK
Copy link
Member

BYK commented Jul 5, 2017

@bestander @arcanis may be we should extend this ignore list via an environment variable so we don't have to keep this manual list in the code?

@arcanis
Copy link
Member

arcanis commented Jul 5, 2017

I'm fine with the manual list in this particular case (I fear allowing to configure this field would trigger non-obvious issues where a package would work on some computers but not others because an ignore key hasn't been set), but if we are to make this configurable, it should probably be through the use of the yarnrc rather than an environment variable.

@BYK
Copy link
Member

BYK commented Jul 5, 2017

but if we are to make this configurable, it should probably be through the use of the yarnrc rather than an environment variable.

Great idea, yeah.

@bytenik
Copy link
Contributor Author

bytenik commented Jul 5, 2017

Prettier doesn't seem to do anything.

yarn prettier v0.27.5
$ node scripts/prettier.js write
Done in 7.03s.

C:\Users\bytenik\AppData\Local\Temp\yarn>git status
On branch patch-1
Your branch is up-to-date with 'origin/patch-1'.
nothing to commit, working tree clean

C:\Users\bytenik\AppData\Local\Temp\yarn>

@BYK
Copy link
Member

BYK commented Jul 5, 2017

@bytenik oh interesting. Then would you mind rebasing onto master or merging from it to see if it would clear the CI failures?

@anpage
Copy link

anpage commented Jul 6, 2017

The linter is failing because the line is too long. The comment needs to be shortened, which could be why yarn prettier couldn't automatically fix it.

@BYK
Copy link
Member

BYK commented Jul 6, 2017

@bytenik you may also us a link shortener like bit.ly

@bytenik
Copy link
Contributor Author

bytenik commented Jul 6, 2017

I took the bit.ly approach.

@BYK BYK merged commit 3901ba4 into yarnpkg:master Jul 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants