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

Naja suport and switched to webpack #740

Closed
wants to merge 7 commits into from
Closed

Naja suport and switched to webpack #740

wants to merge 7 commits into from

Conversation

filipcro
Copy link
Contributor

@filipcro filipcro commented Feb 8, 2019

Javascript assets are now split into multiple files, which are bundled with webpack. (You can use npm run build to build it.) Extensions are all now in separated files. in folder assets\src\extensions. Functions used for sorting, group and multiple selects are in assets\src\actions, and some utils used to proxie nette-ajax to naja are in assets\src\utils.

The code was made by copying transpired Coffeescript. The code was copied and switched to use proxy functions instead of nette-ajax. I did my best to preserve original comments in code, and to improve some obvious generated code weirdness. Event listeners are still in their the original files, and I didn't change original distribution file structure.

#738

@f3l1x
Copy link
Member

f3l1x commented Feb 13, 2019

Hi. Thanks a lot. As you mentioned the code looks weird because it's generated. I must take a look. But it seems like a way to get rid of coffescript. Thank you.

@dakorpar
Copy link
Member

@paveljanda @f3l1x you have some session in plan to solve PRs?

@f3l1x
Copy link
Member

f3l1x commented Mar 11, 2019

@filipcro Hey Filip, which tool did you use for transpilation CS to JS?

I have found this one https://decaffeinate-project.org/. It generates more human readable output. WDYT?

@filipcro
Copy link
Contributor Author

I used default coffescript compiler. It didn't occur to me to search for tool like this one.

@f3l1x
Copy link
Member

f3l1x commented Mar 11, 2019

Could you please take a look and give it a try? Thanks.

@filipcro
Copy link
Contributor Author

I tried it, and it seams to work. I will use it to make better code and push it to this pull request.

@filipcro
Copy link
Contributor Author

So first, I added lint script. We should probably add more rules and add it (npm run lint) to CI.

By using decaffeinate output was much nicer so I use it to generate JavaScript. I test it in my projects and it seams to work like expected with both Naja and nette-ajax.

@f3l1x
Copy link
Member

f3l1x commented Mar 18, 2019

Good job! I was thinking about drop dist scripts at all, WDYT?

@filipcro
Copy link
Contributor Author

Now JS is split into multiple files, and some use require instead JS standard import. (because we can't check if library is included with import ). If you are using webpack you shroud use source files, but for anybody else I'd leave dist scripts.

In next version we could turn dist scripts into one and add some way to configure it.

@dakorpar
Copy link
Member

@paveljanda do U plan to push this to v6.x at least if not with 5.8 as well?

@paveljanda
Copy link
Member

I will discuss this Naja/Webpack support on upcoming Contributte trip. Thanks for reminding me.

@dakorpar
Copy link
Member

@paveljanda will there be any progress on this? it will just get more and more hard to merge, and IMHO this is really needed since nette.ajax.js is abandoned and not maintained, history is not working at all anymore with nette 3 https://github.com/vojtech-dobes/history.nette.ajax.js/blob/master/composer.json
I need some answers to see further plans...

@vojtechmares
Copy link

@dakorpar since v6 coffescript and sass was dropped, bower had stayed.

As you have mentioned it is becoming really hard to merge this PR. I might say that this PR is doomed and I think it would be easier to merge a brand new PR based on 6.1.0 release instead, for example.

Also v6 have introduced quite a few BC breaks, but I encourage everyone to upgrade to v6.

@dakorpar
Copy link
Member

this is really nice, I created issue where from @paveljanda I got yes #738 I paid employee to implement this just so that this PR stays stucked here and we're locked to it on project now.
That really sucks. I allways want to contributte where and when I can but stuffs like this will definitivelly drive me away from project...

@paveljanda
Copy link
Member

@dakorpar

1, I said "yes" to "Do you want datagrid to support Naja?"
2, I understand you disappointment but there is a long list of thing to do on opensource projects. Resolving this pull request needs a lot of time and as you can see, it is a really big pull request with a lot of bc breaks. And that needs time and patience.
3, Thank you for contributing to opensource project, I appreciate that.
4, It is nice that you paid you employee. And what? Should we feel terrible now? Should we feel terrible for providing you an awesome opensource library for free?

Naja support will eventually come when there is time to spent programming opensource. Right now, I focus mainly on fixing issues - this is a top priority for me.

Maybe we can meet with your employee (👍) and discuss Naja support and work on it together to prevent potential losses?

@dakorpar
Copy link
Member

@paveljanda I think you did not understood me. This PR is here for a long time, I asked in issues before going with it. I bumped it few times and here and on slack and no one didn't really do anything on it. This is communication issue. Someone should say it's not in our priority list. We did whole thing, tested it, use it on our projects and someone from maintainers should just check it and merge it, give us some input, but in 4 months nothing happened. You actually went with switching off coffescript (which we did already anyway before) and just completelly ignored this PR.

I do realize this is opensource free library, and I'm gratefull on it and I'm also donating open source libraries on regular basis and also do contributions. But for me this communication was unnaceptable and putted me into a bad position, that's all, and that's why I'm posting comments here.

@paveljanda
Copy link
Member

I didn't really ignored this issue. There was just different priorities. I want a Naja support once. Preferably without BC breaks.

@vojtechmares
Copy link

vojtechmares commented Jun 12, 2019

@dakorpar there is a Roadmap issue for the whole Contributte organization, which was there for 2018 and the 2019 is yet to be announced (@f3l1x).

See contributte/contributte#3 and contributte/contributte#6

Contributte is no longer just about a few packages, but currently 15 team members (including you) maintain over 120 packages (at the time of writing).

At PHP Nomad & 105. PoSobota there was 1/5 of Contributte members (@f3l1x, @paveljanda and me) and there was a big discussion about Contributte's (and Datagrid's) future, monetization, etc.

I see your point in the lack of communication in this PR. But as mentioned before none is being paid to contribute and we are doing it in our spare time. Because of that and in combination with number of packages, to me it seems logical that the development is slow.

I completely agree that there is a lot of space for improvements and that this should be resolved, but this is up to the package maintainer.

@dakorpar
Copy link
Member

I'm not really aware of any BC breaks here since this code worked well for us on both nette.ajax.js and naja... I just wish that communicated was better.
IMHO this just needed some maintainer here to test if everything is fine both on naja and nette.ajax.js, existing functionalities were untouched. Whatever would come up we would solve it.
Only thing that annoys me is that you basically give green light to do this and then went completelly different way making this impossible to merge and this was done before anyone did anything regarding js (switching off coffescript). Anyway, it's done, don't get me wrong I really like this library I'll keep using it and whatever I see I'll try to contributte when I can, like I already did. if U have some patreon or anything I would do some donations as well. Just try to communicate better. I understand that you have meetups and that you're all Czechs so that makes me an outsider, but this is another (huge) issue in whole nette ecosystem

@vojtechmares
Copy link

vojtechmares commented Jun 12, 2019

@dakorpar fair points I would say, but now we are here and the blame to me seems pointless. Let's take it as criticism and as a topic we should improve.

I do not think that anyone in Contributte would have a problem with speaking only in English, but I get it and the meetup was in Czech republic. Maybe we should arrange a bigger discussion or something for example at next Nette Conference (first was in April 2019, I think at 10th of Apr.).

I think that everyone who cares about Nette commnunity sees the problem of being Czech-only for a very long time. And there is effort to make it more open for other people from all around the world.

Nette and Contributte members attended to PHP Barcelona and even had a talk there. But that is not enough, I know.

We should produce more content in English, but it is easy to say rather then creating it.

But this is becoming offtopic, let's continue in different place e.g. Nette Forums or new issue under contributte/contributte. I would love to hear your thoughts, since you are part of contributte, reach me at our Slack :)

Edit: just a side note @dakorpar I tweeted to you on twitter to send me your email address in PM so I can add you to Contributte's Slack

@paveljanda
Copy link
Member

@filipcro Could you possibly change this PR into a naja branch? I finally learned a piece of js modules, npm and Naja. I would like to use you pull request although I would alter it a bit.

naja branch is branched from master. You don't have to make it completely functional, just resolve possible conflicts. Thanks a lot!

@filipcro
Copy link
Contributor Author

I'll check it over the weekend, and I'll make PR.

@dakorpar
Copy link
Member

@paveljanda you can close this

@f3l1x
Copy link
Member

f3l1x commented Dec 5, 2020

Thank you all for your work. Closing it here, because it's already implemented (Naja).

#927

@f3l1x f3l1x closed this Dec 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants