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 frontend assets being outdated (Lombiq Technologies: OCORE-158) #15735

Merged
merged 75 commits into from
Apr 18, 2024

Conversation

Piedone
Copy link
Member

@Piedone Piedone commented Apr 11, 2024

Fix #15726
Fix #11916
Fix #7835

Checked the login, admin, and Trumbowyg editor, and they all appear to be fine still.

@Piedone
Copy link
Member Author

Piedone commented Apr 11, 2024

Thanks for the approve, but I'm not done yet :). The build validation fails still. It turns out that the Gulp build for Trumbowyg plugins is nondeterministic and you can randomly get differently ordered files in the concatenated result. I think I fixed that, but there's still something else, most possibly some line ending difference.

@Skrypt
Copy link
Contributor

Skrypt commented Apr 11, 2024

Everything else pass on CI so approved 😉

@Skrypt
Copy link
Contributor

Skrypt commented Apr 11, 2024

Hey Zoltan, I have a new asset builder that can make use of Parcel, Webpack, Rollup and that should eventually work better than this gulp file.

@Piedone
Copy link
Member Author

Piedone commented Apr 11, 2024

The long-term solution I'd argue would be to build static files on build too. However, that needs all contributors to go through the painful setup of Node, even if they never touch static files. This is what we do with Node.js Extensions though.

Can you please elaborate? Do you have a PoC somewhere?

gulpfile.js Outdated Show resolved Hide resolved
@Piedone Piedone requested a review from MikeAlhayek April 17, 2024 20:55
Copy link
Member

@MikeAlhayek MikeAlhayek left a comment

Choose a reason for hiding this comment

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

LGTM! Good work!

@Piedone
Copy link
Member Author

Piedone commented Apr 17, 2024

Thank you! @Skrypt would you like to add anything, or veto merging this?

@MikeAlhayek
Copy link
Member

In the US, we like to use veto just because :).

Given that last comment @Skrypt had I veto to merge it so the 1.9 milestone list will shrink

Okay, that's to much politics

@MikeAlhayek
Copy link
Member

image

@MikeAlhayek MikeAlhayek merged commit 9688beb into OrchardCMS:main Apr 18, 2024
5 checks passed
@Piedone
Copy link
Member Author

Piedone commented Apr 19, 2024

Seems like a political move :D.

@MikeAlhayek
Copy link
Member

we'll when you have the right to veto, then you can abuse it's power and no one can veto your veto 😜

@Piedone
Copy link
Member Author

Piedone commented Apr 19, 2024

image

@Skrypt
Copy link
Contributor

Skrypt commented Apr 19, 2024

Are you guys trolling me there?

@Piedone
Copy link
Member Author

Piedone commented Apr 19, 2024

I'm joking with Mike, but I genuinely asked for your feedback under #15735 (comment). Certainly no offense intended. Sorry if it came over like that.

@Skrypt
Copy link
Contributor

Skrypt commented Apr 19, 2024

Yeah, it sounded like if I was abusing my contributor role by blocking PR's and altering them to meet some requirements.
I never had that issue here in 10 years?

@MikeAlhayek All of this starts with trolling me on : "should I test this?" on meetings, PR's and then here by making allusions. I don't like where this discussion is going to be honest. I was really not making a joke when I said last week that it is maddening when you need to clean up everyone's PR after they get merged. This is why we generally block them.

Apologies accepted but if there's anything else please do it in private.

Thanks

@Piedone
Copy link
Member Author

Piedone commented Apr 19, 2024

It wasn't really meant against you like that. Let me try to clear it up; I'll kill the joke by explaining it (including Mike's part, how I understand it, but of course I don't speak for him), but of course, a joke is only good if everyone can laugh at it, and apparently this wasn't a good one. And it's better to talk through conflicts too much than too little.

  1. Since you were involved with this PR (and the matter otherwise) previously, I asked for your for your feedback here: Fix frontend assets being outdated (Lombiq Technologies: OCORE-158) #15735 (comment). I didn't mean anything more with "veto" than a short form of "disagree with merging it".
  2. Given that "veto" is also used in political contexts, and usually negatively (people using it inappropriately), Mike jokes with it (the word, not you vetoing the PR) here, also jokingly threatening to veto himself (mimicking how people misuse vetos): Fix frontend assets being outdated (Lombiq Technologies: OCORE-158) #15735 (comment)
  3. From Fix frontend assets being outdated (Lombiq Technologies: OCORE-158) #15735 (comment) we continue to banter with Mike on vetoing, while not having much to do with the PR and especially not with you.

Mike, please add anything if you deem necessary.

I hope this clears things up :). The lesson learned here is probably that we should stick to jokes that are understandable for everyone within this professional context, and leave out the memes even slightly related to politics.

I don't know about the meeting you refer to, so I'm probably missing some context here that shines a different light on the above exchange, but as for me, it didn't really have anything to do with it. Can you tell which meeting that was BTW? If a Tuesday one, then I'll watch the recording.

And I think that your work as a core contributor is very valuable. If I ever find a more general issue with your activities then I'll be sure to bring that up privately and directly, not publicly and in such roundabout ways. I ask everyone to do the same for me too.

@Skrypt
Copy link
Contributor

Skrypt commented Apr 19, 2024

Ok thanks, I'm just making links where I shouldn't. I appreciate the explanation.
Have a good day, (at least better than yesterday) 😄

@Piedone
Copy link
Member Author

Piedone commented Apr 19, 2024

Glad to hear we have it settled!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Frontend assets are outdated Run gulp pipeline as part of the CI [CI] Add schedule run for manual tests
3 participants