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

Jump to bottom

--copy-ignored flag added to CLI #10887

Merged
merged 13 commits into from Jan 10, 2020
Merged

--copy-ignored flag added to CLI #10887

merged 13 commits into from Jan 10, 2020

Conversation

rajasekarm
Copy link
Member

Q                       A
Fixed Issues? Fixes #6226
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature? Yes
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

@rajasekarm rajasekarm added this to the v7.8.0 milestone Dec 18, 2019
@rajasekarm rajasekarm added pkg: cli PR: New Feature 🚀 A type of pull request used for our changelog categories labels Dec 18, 2019
@rajasekarm rajasekarm changed the title Reopen - Prevent ignored files in out dir Reopen - includeIgnored flag added to CLI Dec 18, 2019
@nicolo-ribaudo
Copy link
Member

Reviews at #10831

JLHwung
JLHwung reviewed Dec 18, 2019
View reviewed changes
packages/babel-cli/src/babel/dir.js Outdated
if (
!written &&
((!isCompilableExtension && cliOptions.copyFiles) ||
cliOptions.includeIgnored)
Copy link
Contributor

@JLHwung JLHwung Dec 18, 2019 •
edited

Choose a reason for hiding this comment

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

I think we should throw in parseArgv when copyFiles: false, includeIgnored: true, otherwise we will have another flag that could copy non compilable files.

Copy link
Member

Choose a reason for hiding this comment

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

🚲🏠 warning:

Should we call it --copy-ignored? 😝

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to --copy-ignored

JLHwung
JLHwung reviewed Dec 18, 2019
View reviewed changes
...ages/babel-cli/test/fixtures/babel/--copy-files with ignore with includeIgnored/options.json Outdated
@@ -0,0 +1,12 @@
{
"args": [
Copy link
Contributor

@JLHwung JLHwung Dec 18, 2019 •
edited

Choose a reason for hiding this comment

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

Can you add a test for --only?

babel src --out-dir lib --copy-files --only src/main/* --include-ignored

and a test when --include-ignored is not enabled (default of v7.8), so that we can make sure the old behaviour preserves.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this still needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good to have.😀

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a test case for this.

https://github.com/rajasekarm/babel/tree/fix/10829/packages/babel-cli/test/fixtures/babel/error%20--copy-files%20--only%20--copy-ignored

@rajasekarm
Copy link
Member Author

I'm throwing error if --copy-ignored is used without ignore flag. Which will solve other corner cases.

@rajasekarm rajasekarm changed the title Reopen - includeIgnored flag added to CLI Reopen - --include-ignored flag added to CLI Dec 20, 2019
nicolo-ribaudo
nicolo-ribaudo previously approved these changes Dec 20, 2019
View reviewed changes
@nicolo-ribaudo nicolo-ribaudo added PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release and removed PR: Ready For Review labels Dec 20, 2019
@nicolo-ribaudo nicolo-ribaudo removed the request for review from JLHwung December 20, 2019 23:38
JLHwung
JLHwung reviewed Dec 22, 2019
View reviewed changes
...li/test/fixtures/babel/--copy-files --include-dotfiles with ignore/out-files/lib/foo/.foo.js Outdated
@@ -1 +0,0 @@
a;
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔Why is .foo.js not copied?

Copy link
Member Author

Choose a reason for hiding this comment

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

we've --include-dotfile to Include dotfiles when compiling and copying non-compilable files.

https://github.com/rajasekarm/babel/blob/master/packages/babel-cli/src/babel/options.js#L148

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it was copied before this PR. But now .foo.js has been removed from the out files, which means it is not copied.

Copy link
Member Author

Choose a reason for hiding this comment

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

I got it, I'll check.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've that file in my branch, https://github.com/rajasekarm/babel/blob/master/packages/babel-cli/test/fixtures/babel/--copy-files%20--include-dotfiles%20with%20ignore/out-files/lib/foo/.foo.js

I dono why github shows this as removed.

Copy link
Member

Choose a reason for hiding this comment

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

The linked one is your master branch, not the PR branch 😛

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'm check this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Found the root cause of this bug, I'm fixing it.

Copy link
Member Author

Choose a reason for hiding this comment

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

.foo.js is inside src/foo and it is in ignored list. So it is not copied.

@rajasekarm
Copy link
Member Author

I've exposed pathToPattern function from @babel/core for the ignore file check.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Dec 25, 2019 •
edited

I've exposed pathToPattern function from @babel/core for the ignore file check.

I have bad news for you 😛
This is a breaking change because if someone used @babel/core v7.7.7 and updates @babel/cli to 7.8.0 won't work.
For now, we can duplicate the code (with comments to explain that it's duplicated and if it is updated it should be done in both places) and then revisit it for Babel 8.

@rajasekarm
Copy link
Member Author

Sure, I'll make the change.

@rajasekarm
Copy link
Member Author

@nicolo-ribaudo Done 👍

@nicolo-ribaudo nicolo-ribaudo changed the title Reopen - --include-ignored flag added to CLI Reopen - --copy-ignored flag added to CLI Dec 25, 2019
@nicolo-ribaudo nicolo-ribaudo dismissed their stale review December 25, 2019 23:37

(I need to review this PR again)

@nicolo-ribaudo
Copy link
Member

I was looking at the write function in packages/babel-cli/src/babel/dir.js. From what I understood, it can return false in three cases:

  1. The extension is not compatible (first return false)
  2. The file has been ignored, and thus babel.transformSync (aka util.compile) returns null
  3. There was an error.

If we make the write function return a string enum rather than a boolean, maybe we could avoid checking if the file has been ignored? Also, I think that the current approach doesn't work well if a file has been ignored not using a cli option, but using a config file.

@rajasekarm rajasekarm changed the title Reopen - --copy-ignored flag added to CLI copy-ignored flag added to CLI Dec 27, 2019
@rajasekarm rajasekarm changed the title copy-ignored flag added to CLI --copy-ignored flag added to CLI Dec 27, 2019
@rajasekarm
Copy link
Member Author

rajasekarm commented Dec 27, 2019 •
edited

@nicolo-ribaudo I made the requested change. Can you please check?

nicolo-ribaudo
nicolo-ribaudo approved these changes Dec 27, 2019
View reviewed changes
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

The code now looks way cleaner!

This was referenced Mar 14, 2020
Merged
Open
Merged
Merged
Open
Open
Open
This was referenced Mar 22, 2020
Open
Open
Open
Open
Open
Open
Open
Open
This was referenced Mar 29, 2020
Open
Open
Open
Open
Open
Open
This was referenced Apr 5, 2020
Open
Open
Open
Open
Open
Open
Open
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Apr 24, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@existentialism existentialism existentialism approved these changes

@JLHwung JLHwung JLHwung approved these changes

@nicolo-ribaudo nicolo-ribaudo nicolo-ribaudo approved these changes

@kaicataldo kaicataldo kaicataldo approved these changes

Assignees
No one assigned
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: cli PR: Needs Docs PR: New Feature 🚀 A type of pull request used for our changelog categories PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release
Projects
None yet
Milestone
v7.8.0
Development

Successfully merging this pull request may close these issues.

copy-files flag does not respect ignore block in .babelrc
5 participants
@rajasekarm @nicolo-ribaudo @JLHwung @existentialism @kaicataldo