-
Notifications
You must be signed in to change notification settings - Fork 34
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
Merge changes from Sugarizer's web library #133
Conversation
@cheongsiuhong can you share your test methods? |
I manually ran every Sugarizer activity with their default |
I'm guessing you ran them in sugar, if that's the case; how did you run it? |
The Sugarizer activities were tested in Sugarizer. If you're asking how to run Sugarizer activities in Sugar, you can run |
Any changes made in The changes can't be tested on sugarizer activities in sugarizer as it's an update for You can read more at the issue you referenced. |
Nice work. Just need some way to test that everything is working in Sugar before we merge it. Is there a test method? Is it as simple as replacing the directory in an already working Sugar activity? |
Unfortunately, there are a fair number of issues that make this pull request somewhat hard to test properly. The Karma tests don't work, probably due to the dependency being very outdated. As the purpose of this patch was solely to port changes over from Sugarizer, I avoided changing any dependencies. Secondly, the Sugar web activities that are currently on the sugarlabs GitHub mostly use an outdated version of this library. It took some time, but I tested all of the activities here with this updated version of sugar-web. The Moon activity is the only one that uses a somewhat up to date sugar-web library, and it works with no issue when I replaced it with the updated one. It is unclear if the problems lie with this patch, or with the outdated activities that may not be have been written to work with the current version (meaning unpatched) of sugar-web. It would take quite abit of time to debug and fix them all, if that were the case... Hope to hear some guidance on this issue. edit: some issues are caused by |
The challenges you've faced are expected, some problems might lie with the patch and some problems might not, you'll have to figure out which problems are which and try fixing them as you go along. In the case of some activites |
Thanks @cheongsiuhong and @chimosky. I urge caution in using Moon as an example, because I made that release without much experience, and also how it can be used to induce a specific failure in the journal interaction. Yes, the GitHub Sugarlabs repositories are outdated, because (a) we have had no JavaScript maintainers for Sugar, and (b) maintainers for Sugarizer have forked the source code into the Sugarizer repository. (Which has reminded me to pull from @llaske and push to @sugarlabs following the most recent release - done). Sugar web has been under maintained. There is a 0.100 branch, a 0.102 branch, and then a v0.110.0 tag but no later. Sugar is at 0.117 now. Guidance;
If you agree on this plan, then I suggest it be posted on sugar-devel@ mailing list, because the audience for this repository is limited. |
@quozl I suggest also to update file one by one instead of updating all files at the same time. When I've forked sugar-web I've isolated the initial source code an create sub directories for each file with the Sugarizer code in a For example, here is the code in the
I can't guaranty that this logic has persisted since the beginning but with this, it should be easier to update Sugar-Web. |
Thanks @llaske! Yes, that seems like it would be an effective way to iterate. |
Thanks all for the feedback.
Currently, I think I'd work on the above steps. I agree with @llaske that pushing incrementally would make the updates easier to review, but it may be quite challenging to test the functionality of each iteration. It would then require something like:
Would this be a suitable workflow? |
Thanks. That workflow seems fine to me. Another would be to add a test activity inside sugar-web or somewhere else that can serve as a worked example and test for the API. Like Hello World but using all API features. |
For work on incrementally updating sugar-web, I will open different pull requests for each separate file change. Closed this for now. |
Part of #127 .
As per title, this pull request merges changes from Sugarizer's web library back to this repository.
Currently, running
volo add -f
from a Sugarizer activity causes it to pull sugar-web from this repository. However, this library is outdated and doesn't work with Sugarizer activities.The changes accomplish the following:
env.isSugarizer()
to run different branches of code on the different platforms. In other words, the library is still essentially the same when running on Sugar Desktop.What doesn't work:
grunt test
is broken.Most of the code written is by @llaske, with some fixes made by me during the porting.
The changes made are fairly extensive, and difficult to compare due to some files being moved around, particularly for
datastore
andbus
. This is quite a major upheaval to this library, and I hope to get feedback from you guys.