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(compiler) restore old copy behaviour, with new ignore option #6020

Merged
merged 1 commit into from
Nov 8, 2024

Conversation

George-Payne
Copy link
Contributor

@George-Payne George-Payne commented Oct 8, 2024

What is the current behavior?

#5899 introduced a lot of regressions:

A breaking change in copy behavior: #5983
file to file copying broken: #5956
EBUSY: resource busy: #5966
Directories from above the root dir are copied outside of the output dir.

What is the new behavior?

This PR restores the old copy behavior, but uses the passed filter list, rather than the hardcoded one.

This isn't an optimum fix, as it changes the copy back to dir to dir (or file to file), so ignore patterns aren't matched against child files. Only glob matches are ignored (as was the previous behavior). IMO it's better than the large number of regressions, especially as this can now be worked around (e.g. in the case of #5781) by the user moving to a glob copy, with the new ignore option.

Documentation

none

Does this introduce a breaking change?

  • Yes
  • No

Testing

Tested on own repo.

Other information

fixes: #5983
fixes: #5956
fixes: #5966

@dutscher
Copy link
Contributor

dutscher commented Oct 9, 2024

we encounter copy problems with "@stencil/angular-output-target": "^0.9.0", "@stencil/react-output-target": "^0.5.3", after update to 4.21.
i guess this restore will solve this issue.
cheers

@Armand-Lluka
Copy link

Hey, is there any update or traction on this? This is currently acting as a blocker from us updating.

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Sorry for this regression. We were not completely aware of all use cases. This will help us fix the current issues and eventually attempt to get this right a second time.

@christian-bromann
Copy link
Member

@George-Payne it seems like the copy task test I added is failing, mind checking?

@George-Payne
Copy link
Contributor Author

George-Payne commented Oct 28, 2024

it seems like the copy task test I added is failing, mind checking?

@christian-bromann Didn't realise there were multiple test commands, so missed that one. I moved it to use a glob and amended the commit.

@Armand-Lluka
Copy link

Armand-Lluka commented Nov 8, 2024

Hey, is there anything blocking this from being added? We're very excited about this fix to make it in 😃

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@christian-bromann christian-bromann added this pull request to the merge queue Nov 8, 2024
@christian-bromann
Copy link
Member

@George-Payne thanks for the contribution, we will release this next week on Monday.

Merged via the queue into ionic-team:main with commit 4cabb30 Nov 8, 2024
88 checks passed
@George-Payne George-Payne deleted the old-copy-behaviour branch November 11, 2024 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants