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

Extract leancloud-counter to plugins (Reopening #677) #707

Closed
wants to merge 9 commits into from
Closed

Extract leancloud-counter to plugins (Reopening #677) #707

wants to merge 9 commits into from

Conversation

LEAFERx
Copy link
Contributor

@LEAFERx LEAFERx commented Mar 18, 2019

Previous discussion is at #677

@ivan-nginx
Copy link
Member

How you create draft pull? oO

@LEAFERx
Copy link
Contributor Author

LEAFERx commented Mar 19, 2019

@ivan-nginx https://help.github.com/en/desktop/contributing-to-projects/creating-a-pull-request Step 5

@LEAFERx
Copy link
Contributor Author

LEAFERx commented Mar 29, 2019

Hey guys. Here's a little thing:
I have refactored the on-page script. It is an aggressive one that gets rid of jquery, exposes API for expansion, refactors DOM selection for less specific DOM dependent, etc.
I have also provided the legacy script in the plugin, and for now, in this pr, it uses the legacy script for compatibility of Valine counter.
Do you think I should remain it this way and make pr later or refactor theme-next DOM to use the new script just in this pr?

Also, I think this pr could catch up v7.1.0 milestone as long as we decide now

@1v9
Copy link
Member

1v9 commented Mar 29, 2019

Breaking change?✔

@stevenjoezhang
Copy link
Contributor

It sounds exciting! However, there is not much time left in this month... you can open a PR, and let's discuss the future of NexT together

@LEAFERx
Copy link
Contributor Author

LEAFERx commented Mar 29, 2019

It sounds exciting! However, there is not much time left in this month... you can open a PR, and let's discuss the future of NexT together

@stevenjoezhang oops, I guess you misunderstand me. I mean refactored the leancloud counter script.

Here is what i want to discuss: Are we going to use the legacy script for now (this pr) or just use the new script in this pr (cause DOM will change a little)

@stevenjoezhang
Copy link
Contributor

Ahh... I think both is OK

If refactoring theme-next DOM could make things clearer, it would be better to do make the changes

I'm not sure if one day NexT will fully gets rid of jQuery... it will take even longer time to do the refactoring

@ivan-nginx
Copy link
Member

ivan-nginx commented Mar 29, 2019

I'm not sure if one day NexT will fully gets rid of jQuery...

I'm not sure about this too, but it may be in future.

@LEAFERx make a decision by yourself for the best variant. Better, if module (or scripts) will not depends on other libraries.

@LEAFERx
Copy link
Contributor Author

LEAFERx commented Mar 29, 2019

Finally it's tested and ready I think. Guys, please review it.

Also, hexo-leancloud-counter may still need to polish some details (LEAFERx/hexo-leancloud-counter#3). But it's good enough to go, at least changes will not effect next cause this pr is enough for next now.

Next pr for next may be the live hot post feature.

@LEAFERx LEAFERx marked this pull request as ready for review March 29, 2019 16:47
@ivan-nginx
Copy link
Member

@ivan-nginx yes see theme-next/theme-next.org#32

I mean full docs on NexT website. Like as Algolia search. We need to remove all excess docs from this NexT repo, so, they must be on website only.

@LEAFERx
Copy link
Contributor Author

LEAFERx commented Mar 30, 2019

Note that this is an independent plugin. So all docs that the theme need to do is already in the pr. But i don't think it's a good choice to move https://theme-next.github.io/hexo-leancloud-counter/ to https://theme-next.org
One is theme one is plugin. NexT integrate plugin but not contain plugin. What do you think?

@ivan-nginx
Copy link
Member

ivan-nginx commented Mar 30, 2019

  1. Algolia (as an example) independent plugin too. But NexT site contains full docs how to set it up step by step.
  2. If plugin used by NexT, I think NexT must explain step by step how to set it up (e.g. see above).
  3. You posted LC documentation to https://theme-next.github.io website, so, what's difference? I think if u post it to the NexT official site, peoples will be think «That's alraight, it's official and must work!», but if we will stay it as now, peoples will think «Why next docs not exists, but just linked on another, non-official theme-next.github.io site? It's strange, I think I don't use this plugin».

And if u think about languages switcher, don't worry about it, I'll add it soon.

@ivan-nginx
Copy link
Member

ivan-nginx commented Mar 30, 2019

Ok, I can suggest to use subdomain for that docs: leancloud-counter.theme-next.org or something like that. How about this?


But I now think it's bad idea.

@stevenjoezhang
Copy link
Contributor

Maybe not all the docs, just part of it that related to NexT. Because this plugin can be used in other themes.

@ivan-nginx
Copy link
Member

Of course just part to the NexT.

@ivan-nginx
Copy link
Member

ivan-nginx commented Mar 30, 2019

Also, IMHO this new plugin too complicated, than was before. It's just security for counter, it's not comment system or search service...
Most peoples don't know about this bug, but who know – they don't care. Who will spend him time and count / uncount LC on one or another blog? This is a little bit stupid, I think. Because this just counter for vision and it's not affects on SEO, for example.

@LEAFERx
Copy link
Contributor Author

LEAFERx commented Mar 30, 2019

Well I have to explain something.

Why I make this plugin?

First of all, it's a hexo plugin, not a NexT plugin. And I make this plugin not for security.
The initial idea that I begin to make this plugin is hexo-leancloud-counter-security is too much complicated for setup. Yes, normal people don't even care this security problem, so why they need to fix it by instruction of docs/LEANCLOUD-COUNTER-SECURITY? I have meet a lot of questions about details of the instructions and well what if LeanCloud changes its UI? I have to make an other one instruction, and it will also be complicated to write (for me) and follow (for user). So I am thinking about automaticly setup. First I think I just add features to hexo-leancloud-counter-security and it's ok. But I finally choose to write a new plugin. Why?

  1. I want to change the situation about there's so many things that highly-coupled to the theme. People are scared to see the tooooo long and complicated _config.yml (Actually even me will spend a long time to merge the new config and find where to switch features). I wanna change this, at least begin with this counter. NexT can live without this counter right? So why don't we move this out? If we move this thing out, we can 1 reduce the size of NexT (remove of lean_analytics.swig) 2 shorten NexT _config.yml 3 easier (only a lllllitle though) to maintain NexT because less thing need to worry. Also, the counter part is easier to be maintained and upgrade 1 who want to write js in swig? 2 I can easily update counter part and users don't need to wait after new version of NexT. SO see? Do I want to enforce security in new plugin? NO, I don't even mention it in the plugin. And as you can easily induce, if you change the counter word to any other stuff that NexT can live without, it still make sense! So I spend a lot of time thinking, how to extract thing from NexT, how to make NexT simply again. The way is very long but we can make it step by step, and I think the counter will be the first little step.
  2. I still remember #279 and I want to implement features based on this counter. But I DON'T want to write and test thing in swig. And the old code is terrible. If I extract this thing, I can have a lot better developing and testing environment.
  3. We can take advantages of npm, the existed good package management system, and hexo plugin system, to manage NexT.

Is this bunch of thing complicated?

Yes, it's complicated for me. I spend times to figure out architectures, I read a lot of existed hexo plugin code and read hexo code.
But it's not complicated for NexT and user, at least simpler than hexo-leancloud-counter-security. Why? It decrease coupling for NexT and automatic-ify the setup procedure for user. It is also full of maintainability and extendibility. All NexT need to do is implement slots to insert features from the plugin. Assume I implement the hot post feature, NexT only need provide slots (by using helpers), let plugin decides what will fill the slots.

About place of docs

Well I still think it's the best way to put docs. It's a completely independent hexo plugin that can be easily by themes, not only NexT. But your words make some sense, so if you insist, let's pause this merging and have more discussion.


In conclusion, my thoughts are: fully extract things out of NexT -> make it universal -> let NexT integrate it. I hope this can be a little exploring example for modulize NexT.

(too long.... and repitition in words... sorry for that(((

@ivan-nginx
Copy link
Member

Yeah, many words for say same things: independent hexo plugin.

I'll try to rephrase all this discussion in shorter words:

  1. When I say complicated, I mean: plugin have more docs, code and questions, than he actually does. For example, hexo-symbols-count-time provides no less functionality from users point of view; but in the same time he has simple readme, installation and usage instructions (for all themes too, not only for Hexo). Moreover, hexo-leancloud-counter-security have simple instructions too. I'm just a little in shock, why u create separated domain with docs just for plugin like that. Plugin for Hexo, but readme to this plugin on non-Hexo platform...
  2. About #279 – I don't think it's good idea to make feature like this just on LC service. Take it mind, LC – mostly for Chinese users; non-Chinese guys (like me) don't use this service, u know why? Take a look below:
    [ivan-nginx@localhost ~]$ ping leancloud.cn
    PING leancloud.cn (117.50.12.165) 56(84) bytes of data.
    64 bytes from 117.50.12.165 (117.50.12.165): icmp_seq=1 ttl=43 time=298 ms
    64 bytes from 117.50.12.165 (117.50.12.165): icmp_seq=2 ttl=43 time=288 ms
    64 bytes from 117.50.12.165 (117.50.12.165): icmp_seq=3 ttl=43 time=289 ms
    64 bytes from 117.50.12.165 (117.50.12.165): icmp_seq=4 ttl=43 time=289 ms
    ^C
    --- leancloud.cn ping statistics ---
    12 packets transmitted, 12 received, 0% packet loss, time 24ms
    rtt min/avg/max/mdev = 288.250/289.920/298.212/3.098 ms
    [ivan-nginx@localhost ~]$ ping theme-next.org
    PING theme-next.org (142.93.108.123) 56(84) bytes of data.
    64 bytes from 142.93.108.123 (142.93.108.123): icmp_seq=1 ttl=56 time=70.4 ms
    64 bytes from 142.93.108.123 (142.93.108.123): icmp_seq=2 ttl=56 time=62.6 ms
    64 bytes from 142.93.108.123 (142.93.108.123): icmp_seq=3 ttl=56 time=62.4 ms
    64 bytes from 142.93.108.123 (142.93.108.123): icmp_seq=4 ttl=56 time=69.3 ms
    ^C
    --- theme-next.org ping statistics ---
    4 packets transmitted, 4 received, 0% packet loss, time 7ms
    rtt min/avg/max/mdev = 62.368/66.182/70.419/3.710 ms
    [ivan-nginx@localhost ~]$ ping google.com
    PING google.com (172.217.168.238) 56(84) bytes of data.
    64 bytes from ams15s40-in-f14.1e100.net (172.217.168.238): icmp_seq=1 ttl=58 time=44.6 ms
    64 bytes from ams15s40-in-f14.1e100.net (172.217.168.238): icmp_seq=2 ttl=58 time=45.4 ms
    64 bytes from ams15s40-in-f14.1e100.net (172.217.168.238): icmp_seq=3 ttl=58 time=44.6 ms
    64 bytes from ams15s40-in-f14.1e100.net (172.217.168.238): icmp_seq=4 ttl=58 time=44.7 ms
    ^C
    --- google.com ping statistics ---
    4 packets transmitted, 4 received, 0% packet loss, time 7ms
    rtt min/avg/max/mdev = 44.579/44.822/45.433/0.385 ms
    [ivan-nginx@localhost ~]$
    It's stats for now, this stats was in 2016 year. Also, LC interface on CN language too, so, it's impossible to use service like this with some NexT core features as I think. How are u thinking, if I'll start to make important feature for NexT with Yandex API service where all guides on Ru language, are the people will be glad after that? It's rhetorical question, u know.
  3. And about NPM – this is separate talk. Theme under NPM will simple for users, for installations / updates and good for downloads counting; but it will make harder maintenance: when we will need not only publish release in GitHub, but publish NPM package too. Of course, it can be automatize, but we without that have no time for maintenance, have no time for adding / modifying docs, etc., and with each time the whole NexT architecture come more and more complicated. Architecture will not be more easiest, if u will remove 1 swig file from NexT + will add 3x more docs for that plugin, which user will have to read and maybe a little confused about «What I must to do and why so more docs, what's themes? Another themes? What_a_fck, why another domain? Fck this theme, it's complicated for me». 'Keep it simple' – this mean have a bit, but important options which really have weight for user's choose. Options something like «betterPerformance» have no weight, because user will always enable it – that is create complication.

Actually, we can just replace all swig code with js code, but who care about it? If simple user will not see the difference in hes production.
Who will be responsible for this, if ton bugs will be appear after next NexT refactoring?
And, finally, who will waste time hes time on so hard work, just for nothing? Please note, we don’t even get donations for what and how we do, not to mention our time spent.

@LEAFERx
Copy link
Contributor Author

LEAFERx commented Mar 31, 2019

Thank you for your patient reading and response. I understand you. Now I think this plugin may be too complicated for NexT and users. So I guess the best way is to leave it what it is now.

@LEAFERx LEAFERx closed this Mar 31, 2019
@LEAFERx
Copy link
Contributor Author

LEAFERx commented Mar 31, 2019

@ivan-nginx Please, transfer theme-next/hexo-leancloud-counter and theme-next/hexo-leancloud-counter-hookguard to me. Thanks

@ivan-nginx
Copy link
Member

I just requested to add readme for the NexT usage on NexT website. And u write above:

But your words make some sense, so if you insist, let's pause this merging and have more discussion.

So, did you want to continue the discussion? U got. If my words have some sense and u see what I insist on this, why I should to repeat twice? If docs needed, it means they are needed. I waste more time for writing answer to you, than I spend time when I fixed last bug. And if my words and my vision grieved you, then probably exists some reason, and this reason, from my point of view, does not apply to me.

@ivan-nginx Please, transfer theme-next/hexo-leancloud-counter and theme-next/hexo-leancloud-counter-hookguard to me. Thanks

No problem, dude. As I right understand, u want to exit from NexT maintenance?

@1v9
Copy link
Member

1v9 commented Mar 31, 2019

@ivan-nginx I don't think @LEAFERx wants to quit. All of you have discussed enough and I agree with LEAFERx. Let's make things simpler, an independent & universal Hexo plugin is great, all we have to do is provide the necessary doc (on NexT) to the user, if no more conf then no more doc. It seems that I am repeating nonsense 😂

@LEAFERx
Copy link
Contributor Author

LEAFERx commented Mar 31, 2019

My understanding is that hexo-leancloud-counter-security is more suitable than hexo-leancloud-counter for NexT. So there's no need to change a lot. I will continue to maintain hexo-leancloud-counter-security. You may be right, there's no need to waste time to change soo many stuff and bother users. I shall work on hexo-leancloud-counter-security and make users fell no change. Let leave leancloud counter what it is looks like now.

Transfer hexo-leancloud-counter back to me means it is not suitable for NexT now but I still want to continue developing it. Maybe someday it will fit NexT? Who knows. I'm not quitting.

As for the docs... I still think hexo is more likely a blogging platform than a documentation platform. Yes I can simply write everything in README but I just want to try new thing (smile).

@ivan-nginx
Copy link
Member

all we have to do is provide the necessary doc (on NexT) to the user

Yeah, I repeated it several times, but Leaf want to discussion about this (or I don't understand, what). He closed pull here, closed pull in docs site and want to transfer repos...

Transfer hexo-leancloud-counter back to me means it is not suitable for NexT now

From my point of view it means you didn't want to add documentation to the NexT website, and, as we talking with MiMi above, there is no need to add all documentation. But documentation with links only on 3rd-party theme-next.github.io site – also unacceptable: users will confuse and don't understand why 2 theme-next sites exists. If Leaf don't understand it from first explanation, it's mean he pursues personal gain or / and does not take the NexT seriously.
Of course, I understand u make this plugin so long time, u work on it, read the docs, see various Hexo plugins, but let me say to u something: we here also not adding only badges, you know? Remember how NexT looked in iissnan repo? And which infrastructure he was have? And compare this now – we all working to and nobody of us don't drool in the case of constructive criticism... Read Contribution Guide – maybe u will take for yourself something from it.

Moreover, I still don't understand why need to make so big work for support all Hexo themes. Are the NexT not enough? What themes u see under the Hexo which can have on at least for 50% functionality vis-a-vis NexT? Not exists themes like this. Or if exists, please, write directly here, I would like to see on it. That's the point because of which I was disappointing about. And u know I love NexT, and seems not only I.

1 min. and repos will be transferred.

@ivan-nginx
Copy link
Member

GitHub write me this:
image

Maybe u can try to check your e-mail and accept repo.
Or u also can try to transfer by yourself, because andmin and creator of this repos – you. See here.

@LEAFERx
Copy link
Contributor Author

LEAFERx commented Mar 31, 2019

asd
Actually I can't transfer it too...... idk...

I was focusing on the necessary replacing hexo-leancloud-counter-security you pointed out in preivious discussion and miss about docs.... So I am sorry for any misunderstandings...
Actually I do want to transfer it to theme-next.org domain. Why not? theme-next.org is better then theme-next.github.io, definitely. I just don't know how to do it... What I don't want to do is squeeze all docs into https://theme-next.org/docs/third-party-services/statistics-and-analytics (one single block in one single page). I hope this plugin will have at least an independent page. And also, docs should in plugins repo so no duplicated steps (pr in theme-next.org after plugins change a little) will occur.

As for pursing something... Wow I am surprised. All I want to do is improve leancloud counter and contribute to NexT cause I love it. What benefit can I have? I can't understand.

@ivan-nginx
Copy link
Member

Just add basic docs related to the NexT usage. No need to delete options from config (in docs), for example, and give instaed this link to theme-next.github.io. U can add it something like do with previous deprecated plugin: shorter and important config – to the NexT; all other additions docs – to any you want.

For example:

  1. Plugin installation
  2. Options with NexT usage (related only)
  3. Below in note – link to any u want for additional usage / 3rd-party configuration

This will not confuse users, and, who will need it – he will follow the link.


I don't understand why transfer not working, but when I trying to specify full github path (https://github.com/LEAFERx) – GitHub said what user wasn't find:

image
image

@LEAFERx
Copy link
Contributor Author

LEAFERx commented Mar 31, 2019

OK sure. Next time I will be careful about it.

Maybe you can simply type LEAFERx

@ivan-nginx
Copy link
Member

Maybe you can simply type LEAFERx

GitHub write me this:
image

Maybe u can try to check your e-mail and accept repo.
Or u also can try to transfer by yourself, because andmin and creator of this repos – you. See here.

When I type this.

@1v9
Copy link
Member

1v9 commented Mar 31, 2019

@ivan-nginx
Copy link
Member

ivan-nginx commented Mar 31, 2019

Oh, yeah. Checked box, try to transfer by yourself.


But this write in GitHub:
image
And I still can't to transfer this repo's...

@LEAFERx
Copy link
Contributor Author

LEAFERx commented Mar 31, 2019

All done. Thank you @ivan-nginx @1v9

@1v9
Copy link
Member

1v9 commented Mar 31, 2019

@ivan-nginx The operator and receiver must be one person, then transfer is free.

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

Successfully merging this pull request may close these issues.

None yet

4 participants