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

fix: yeoman-generator fork issue #294

Merged
merged 2 commits into from
Mar 1, 2018
Merged

Conversation

dhruvdutt
Copy link
Member

@dhruvdutt dhruvdutt commented Feb 27, 2018

What kind of change does this PR introduce?

Fixes the issue with yeoman-generator fork. Fixes #291 🐛

  • Update yeoman-generator module path in package.json to match the syntax recommended by npm.
  • Change the module name and usage from "webpack-fork-yeoman-generator" to "yeoman-generator" across all files.

Tests

All tests pass. 💯 Also, did a quick test with:

mkdir test-dir && cd test-dir;
mkdir src && touch src/index.js;
webpack-cli init

(Update): Added a temp eslint-disable-line to pass CI. For more description check comments.

Summary

Also, now we don't need to published the fork separately.

Related commit (0b4269c)

Does this PR introduce a breaking change?
No

@dhruvdutt
Copy link
Member Author

dhruvdutt commented Feb 27, 2018

(Update): Added a temp eslint-disable-line to pass CI.

The issue is with how prettier formats and writes our code which later eslint lints.

When yarn format runs, prettier tries to indent and adds an extra space (highlighted in screenshot) which causes yarn lint to fail throwing no-mixed-spaces-and-tabs

screenshot_20180227_155940

I think we did not catch this earlier because these files were not being changed and hence prettier or eslint might be skipping them while checks but this PR contains change in those files.

I also tried using --use-tabs flag with prettier-eslint-cli command as suggested in their docs but it resulted in the same issue.

screenshot_20180227_155640

@justerest
Copy link

it fixs #291

Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

Remove the eslint disables, and I can LGTM

@evenstensberg
Copy link
Member

  • discard changes to any other module, you can do a force push with lease without precomitting

@dhruvdutt
Copy link
Member Author

dhruvdutt commented Mar 1, 2018

@ev1stensberg Updated. 💯

@evenstensberg
Copy link
Member

Okay nice, so you can reimpl the disables, weird they weren't throwing earlier

@webpack-bot
Copy link

@dhruvdutt Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@ev1stensberg Please review the new changes.

@montogeek
Copy link
Member

@ev1stensberg Please apologies in advance, Why is yeoman necessary in this project? And also why a forked was needed?

@evenstensberg
Copy link
Member

It's to grab arguments from a generator ( the config we're scaffolding ). The fork implements a way to do that, yeoman doesn't support it

@webpack-bot
Copy link

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

@montogeek
Copy link
Member

Isn't it possible to implement it from scratch of using another library that supports it?
What it is exactly doing? Replacing values in templates and creating directory/files?

@evenstensberg
Copy link
Member

Possibly, needs investigation anyhow :(

@evenstensberg evenstensberg merged commit 514a695 into webpack:master Mar 1, 2018
@LukeChannings
Copy link

@ev1stensberg

Is there a way to avoid the custom yeoman-generator dependency? I'm working in an environment that loads all dependencies through a Nexus repo and blocks github.com. The latest webpack-cli is not installable for me because of this.

@evenstensberg
Copy link
Member

I'm having a lot to do until thursday +-, will try to get something up after that. We're gonna use yeoman, but not the fork. Right now, we avoid writing to a temp yeoman-rc file, and get the config from the generator instead. To fix this, we probably need to make a temp config and parse it ( sigh ).

@louisscruz
Copy link

The reliance on the custom yeoman-generator dependency brought me here. It ended up introducing some kind of invalid file to my offline-cache (mirror), so I had to roll back a version.

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.

7 participants