Skip to content

Conversation

@susnux
Copy link
Contributor

@susnux susnux commented Jun 29, 2022

Summary

BaseReader: Use a factory pattern for creating the extension list

Provide functions are neither called on the providing object nor
on the injecting object, so using this within provided functions
will refer to the global this for which $emit is not defined.
This uses a factory pattern so that this is bound to the injecting
object.

Also forwarding the click-link event is needed.

@vinicius73 vinicius73 requested a review from max-nextcloud June 30, 2022 14:00
@vinicius73 vinicius73 added the bug Something isn't working label Jun 30, 2022
@susnux
Copy link
Contributor Author

susnux commented Jul 1, 2022

Rebased

@max-nextcloud max-nextcloud requested a review from mejo- July 1, 2022 10:31
Copy link
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

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

I tested the commit and it indeed fixes the issue in Collectives. Would prefer a second opinion on the actual code changes though.

@mejo-
Copy link
Member

mejo- commented Jul 1, 2022

/compile

Copy link
Collaborator

@max-nextcloud max-nextcloud left a comment

Choose a reason for hiding this comment

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

Code wise looks good to me and makes sense. Have not tested it though.

@mejo-
Copy link
Member

mejo- commented Jul 1, 2022

Thanks so much for looking into this @susnux!

Could you push the compiled JS assets so the CI can run against them?

Also, if you don't mind, you could request to become part of https://github.com/nextcloud (to my understanding it's open to regular contributors). That way you could push your branches directly (no need to maintain a personal fork) and CI jobs would run automatically against your PRs.

@mejo-
Copy link
Member

mejo- commented Jul 1, 2022

/backport to stable24

@vinicius73
Copy link
Member

/compile amend

@mejo-
Copy link
Member

mejo- commented Jul 3, 2022

@susnux unfortunately the node CI job fails because apparently the compiled assets have a diff after being recompiled in the CI.

We have the same issue in our development team and I'm not gonna lie: it's cumbersome and we need a better solution. But for us at least the /compile bot command can solve this by compiling the assets and pushing them to the PR branch. Unfortunately that doesn't work with PRs from external repos. That's why I suggested that you join the Nextcloud organization on Github and push your branches directly to our repo in the future.

I'm really sorry for this inconvenience and I hope that we're a better working solution soon.

Another option would be that one of our team pushes your branch to our repo and creates a PR. Just drop me a note if you prefer this option.

susnux added 2 commits July 4, 2022 08:43
Provide functions are neither called on the providing object nor
on the injecting object, so using `this` within provided functions
will refer to the global `this` for which `$emit` is not defined.
This uses a factory pattern so that `this` is bound to the injecting
object.

Also forwarding the `click-link` event is needed.

Signed-off-by: Ferdinand Thiessen <[email protected]>
Signed-off-by: Ferdinand Thiessen <[email protected]>
@mejo-
Copy link
Member

mejo- commented Jul 4, 2022

So for this PR I just went ahead and opened a new one: #2662. At the same time, you should be invited to the Nextcloud Github organisation now @susnux, so in future things should be easier for you 🙈

@mejo- mejo- closed this Jul 4, 2022
@susnux susnux deleted the fix/base-reader-injection branch July 4, 2022 11:47
@susnux
Copy link
Contributor Author

susnux commented Jul 4, 2022

@mejo-

unfortunately the node CI job fails because apparently the compiled assets have a diff after being recompiled in the CI.

Noticed that too. Even using exactly the same npm and node version as the CI does not work.
After some research I found that editor.js.map contains the path to the source file (absolute path) so for files built on my machine those paths include e.g. my username and that will defnetly differ from the CI build.

This seems related to: webpack/webpack#3603

Also, if you don't mind, you could request to become part of https://github.com/nextcloud (to my understanding it's open to regular contributors). That way you could push your branches directly (no need to maintain a personal fork) and CI jobs would run automatically against your PRs.

That required the invite so thank you for arranging this :)

So for this PR I just went ahead and opened a new one: #2662. At the same time, you should be invited to the Nextcloud Github organisation now @susnux, so in future things should be easier for you see_no_evil

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants