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 jscodeshift import (fixes #1064) #1088

Merged
merged 1 commit into from Oct 2, 2019
Merged

Conversation

ohana54
Copy link
Contributor

@ohana54 ohana54 commented Oct 2, 2019

What kind of change does this PR introduce?
Fixes #1064

Did you add tests for your changes?
no :(
This requires testing the CLI itself (as the issue is in index.ts), and currently there are no such tests. I saw there are other CLI tests (for other commands), but none of them test a command which expects user input.
I found the inquirer-test package which might be helpful, so let me know how you think I should proceed here.

Summary
jscodeshift doesn't expose a default import, changed it to import the exported function correctly.

Does this PR introduce a breaking change?
no

@ohana54 ohana54 requested a review from a team as a code owner October 2, 2019 13:24
@jsf-clabot
Copy link

jsf-clabot commented Oct 2, 2019

CLA assistant check
All committers have signed the CLA.

@ghost
Copy link

ghost commented Oct 2, 2019

There were the following issues with this Pull Request

  • Commit: 50ef0a8
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

1 similar comment
@ghost
Copy link

ghost commented Oct 2, 2019

There were the following issues with this Pull Request

  • Commit: 50ef0a8
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@webpack-bot
Copy link

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

Copy link
Member

@anikethsaha anikethsaha left a comment

Choose a reason for hiding this comment

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

LGTM
Just one thing! Did you make any changes with dependencies in your local setup or maybe version changes?
Not sure why those changes in the lock file came from!

@ohana54
Copy link
Contributor Author

ohana54 commented Oct 2, 2019

Well that's odd, I pushed this by mistake :(
I had no luck with this PR, multiple issues - I'll fix it right away.

Copy link
Member

@anikethsaha anikethsaha left a comment

Choose a reason for hiding this comment

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

Looks good now !

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.

lgtm thank you so much for your PR, and congratulations with your first one! 💙 This leads to a new error, can't Cannot read property 'push' of undefined at findPluginsByName.forEach (/webpack-cli/packages/migrate/uglifyJsPlugin/uglifyJsPlugin.js:68:41). Are you willing to fix this as well?

Open an issue using the example file provided in #1064, explain the issue and submit a PR afterwards!

Thank you so much ❤️

@evenstensberg evenstensberg merged commit 181822f into webpack:next Oct 2, 2019
@ohana54
Copy link
Contributor Author

ohana54 commented Oct 2, 2019

Glad to help :)
Regarding the new error, I didn't get it in my use case, so didn't attempt to fix it.
I'll try to reproduce and fix as well.

@evenstensberg
Copy link
Member

Great, thank yous!

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.

[BUG]: Error Cannot create property 'context' on string 'Error generating AST'
5 participants