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(generators): fix and refactor entry util, add tests #1392

Merged
merged 3 commits into from
Apr 1, 2020

Conversation

knagaitsev
Copy link
Contributor

What kind of change does this PR introduce?
fix, refactor

Did you add tests for your changes?
Yes

If relevant, did you update the documentation?
N/A

Summary

The generators entry util was overly complicated and repetitive. One strange piece of logic in it was this:

n[val].charAt(0) !== "(" &&
. I think this was intended for if the user wanted to provide an entry that would be executed, for example path.join(__dirname, 'index.js'). However, allowing this functionality makes no sense given how the init generator will try to write a file using this path (not what this line executes to, but the actual string "path.join(__dirname, 'index.js')", since this would be what the user provided on command line - an unlikely scenario).

I did the following:

  • removed checks to see if the user provided a path that is meant to be executed as mentioned above
  • made the return value an empty string for single entry if the user provides that (not doing this was a bug)
  • fix the users provided path depending on how they formatted it. If they provide src/index then it will be adjusted to ./src/index.js. But if the user provides ./src/index.js, nothing is changed

Does this PR introduce a breaking change?
Yes, as mentioned above

Other information

@knagaitsev knagaitsev requested a review from a team as a code owner March 31, 2020 20:36
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Thanks

@webpack-bot
Copy link

@jamesgeorge007 Thanks for your update.

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

@evilebottnawi Please review the new changes.

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.

4 participants