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

Shows a warning when adding a version-less plugin from a path. #1021

Merged
merged 3 commits into from
May 9, 2017

Conversation

jamonholmgren
Copy link
Member

Fixes #1006.

image

@skellock
Copy link
Contributor

skellock commented May 9, 2017

I'm:

  • 👍 on the plugin changes
  • 👎 on the warning
  • 👎 on the--yolo option

Strawman-mode Engage!

So, run this: react-native init FunTown

image

See that yellow warning in the middle?

As a person building a new React Native project, that's meaningless.

Here's the same thing with create-react-app:

image

So, if I was given the power to --yolo my way thru it, I'd never do it. It would never fix anything.

What I need is for upstream to fix their stuff. I don't even know how far upstream either.

Jump To Conclusions Mat

And, so this is what this PR is bringing to the table, forcing info on end-users where no action can be taken.

Recommendations

  • Keep the plugin.js.ejs changes. As plugin & boilerplate authors, it's on us to responsibly install our dependencies.

  • Drop the --yolo enforcement.

I could be convinced (or at least be upgraded to 😐 ) to keep the warning (if the wording was more along the lines of

Dearest innocent bystander,

Go to {repo} and file an issue with {ignite-plugin} to tell them to add {version} to their addModule({dependency}) like we recommend in {link to docs}.

-- ignite team

@jamonholmgren
Copy link
Member Author

I'm all with you on this, @skellock, makes sense.

I thought you were for the warning, though...?

image

@skellock
Copy link
Contributor

skellock commented May 9, 2017

I thought i was too.

context.print.warning(`Plugin should specify specific version for NPM module ${moduleName} in addModule call.`)
context.print.warning(`Please add version: "<version here>" or to suppress this warning, pass in yolo: true`)
}
return moduleName
Copy link
Member

Choose a reason for hiding this comment

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

dem multiple returns :)

@skellock knows my skin crawled just now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, okay, I'll change it. 🙄

Sidenote: I like multiple returns -- short circuit the rest of the function if it's not necessary, like a guard statement.

But I'm not dogmatic about it. I don't want your skin crawling. 😁

@GantMan
Copy link
Member

GantMan commented May 9, 2017

I think we're saying "warning" so we can do a deprecation phase. I'm fine if it says what @skellock says, but we need something to identify it's not ok... other than just forcing it.

Or maybe we just force it?

@jamonholmgren
Copy link
Member Author

I'm solving the problem by only outputting the warning if we're adding the plugin from a path, like a plugin author might.

image

Copy link
Member

@GantMan GantMan left a comment

Choose a reason for hiding this comment

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

very nice

@jamonholmgren jamonholmgren merged commit 745b745 into master May 9, 2017
@jamonholmgren jamonholmgren deleted the warn-on-plugin-addModule-no-version branch May 9, 2017 23:22
@skellock
Copy link
Contributor

That's a good compromise. Thank you.

@skellock skellock added this to the 2.0 Beta 10 milestone May 10, 2017
@skellock skellock changed the title Warn on plugin addModule if no version specified Show a warning when adding a version-less plugin from a path. May 10, 2017
@skellock skellock changed the title Show a warning when adding a version-less plugin from a path. Shows a warning when adding a version-less plugin from a path. May 10, 2017
@skellock skellock mentioned this pull request May 25, 2017
21 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants