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

[4.0] Let build tools always start clean #35048

Conversation

dgrammatiko
Copy link
Contributor

Pull Request for Issue # .

Summary of Changes

  • Clean all the Joomla core media folders each time that npm install is called

Testing Instructions

Before applying the patch make a backup of your current media folder
Apply the patch
Run npm install
Compare the backup folder with the current media

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Documentation Changes Required

@wilsonge this can wait till 4.0 is released

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels Aug 5, 2021
@brianteeman
Copy link
Contributor

Considering your previous comment is it safe to remove all of vendor and should it not be vendor\xxxxx

@dgrammatiko
Copy link
Contributor Author

Considering your previous comment is it safe to remove all of vendor and should it not be vendor\xxxxx

Yeah, probably I should target only the vendor folders that Joomla delivers. That said the existing code already discards the vendor folder, so I guess I need to fine-tune that part

@@ -788,6 +788,67 @@
"linkText": "Help me resolve this",
"destFile": "/templates/system/fatal-error.html"
}
}
},
"cleanUpFolders": [
Copy link
Member

Choose a reason for hiding this comment

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

Will someone remember to maintain this hardcoded list if some new extension which has assets will be added or removed in future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is the backbone of the build tools but you're right there are way too many hardcoded things here. Will refactor it so it's easier for maintenance

@RickR2H
Copy link
Member

RickR2H commented Aug 6, 2021

I have tested this item ✅ successfully on a964dec


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35048.

@RickR2H
Copy link
Member

RickR2H commented Aug 6, 2021

I have tested this item ✅ successfully on a964dec

Patch works. On loding the article the description gets truncated to 160 characters.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35048.

@richard67
Copy link
Member

I have tested this item ✅ successfully on a964dec

Patch works. On loding the article the description gets truncated to 160 characters.

@RickR2H Can it be that this second test here was meant for another PR? The comment below your test result doesn't fit to this PR here.

@RickR2H
Copy link
Member

RickR2H commented Aug 6, 2021

@richard67 My browser had some hick ups. Sorry for the confusion.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35048.

@RickR2H
Copy link
Member

RickR2H commented Aug 6, 2021

I have not tested this item.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35048.

@RickR2H
Copy link
Member

RickR2H commented Aug 6, 2021

@dgrammatiko how can I verify that the patch works? Should there be a difference between the backed up folder and the newly generated?


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35048.

@dgrammatiko
Copy link
Contributor Author

how can I verify that the patch works? Should there be a difference between the backed up folder

There should be identical (assuming that the backup didn't had .gz files, if it did you need to run the npm run compress iirc)

@dgrammatiko
Copy link
Contributor Author

This PR is not needed urgently so I will switch it to draft and we can check it back after the GA

@dgrammatiko dgrammatiko marked this pull request as draft August 6, 2021 08:51
@dgrammatiko
Copy link
Contributor Author

@RickR2H @richard67 I've opened a new PR with the code from this one plus a replacement of a node package for the watch functionality. Please test #35198

@dgrammatiko dgrammatiko deleted the 4.0-dev-build-tools-rimraf-folders branch August 17, 2021 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NPM Resource Changed This Pull Request can't be tested by Patchtester
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants