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

Remove the utility methods built-in jsblocks. Leave that to lodash and underscore #53

Open
astoilkov opened this issue Jun 15, 2015 · 19 comments
Milestone

Comments

@astoilkov
Copy link
Owner

jsvalue - the built-in utility methods in jsblocks are having more problems than solving ones. The best option is to remove the utility methods by default and release it as a separate library. Only if it becomes stable enough it could get part of some jsblocks built. It should be people decision by default which library to use. Also jsblocks will introduce integration with lodash and underscore by default so the same experience is achieved when using those libraries.

@Kanaye
Copy link
Collaborator

Kanaye commented Oct 30, 2016

What needs to be done here?
I would move the jsvalue-built to a seperate grunt task so we could build it for testing but don't build it in the default tasks.
And I would make the 'mvc' build the default (replacing the blocks.js/blocks.min.js and remove the mvc folder).
Also I would move/reimplement some methods we currently require within jsblocks to jscore (e.g. blocks.toObject is currently required but implemented within jsvalue).

Offtopic:
@astoilkov I sent you some message at gitter last week. It looks like you aren't active at gitter. Please tell me if you have a prefered way to communicate.

@astoilkov
Copy link
Owner Author

Hi,

I would move the jsvalue-built to a seperate grunt task so we could build it for testing but don't build it in the default tasks.

Why we need that? Can't we move that to the jsvalue project?

The other things seems all we need to do. You could do this work in a separate branch and then we should ensure the next version release is not a patch.

Regarding the communication. Gitter is a little buggy so I don't like it much. I use email, facebook messenger, viber. You could choose one and we could connect there.

@Kanaye
Copy link
Collaborator

Kanaye commented Oct 31, 2016

Hi 😄 ,

Why we need that? Can't we move that to the jsvalue project?

I mean the integration task (the once that currently built the blocks-source.js and blocks-query-data.js.

But we of course we can remove these steps complete until jsvalue gets stable enough (a long way but some of the features of jsvalue are awesome so I would love to integrate it again in the future).

I'll start a new branch no-value when I start to work on this.

Regarding communication: I'm neither using facebook nor viber so I guess I'll write you an email once I've thought about some other things some more 😄.

@astoilkov
Copy link
Owner Author

Ok. I understand now. When you start working on this we can figure out the details if needed.

@Kanaye
Copy link
Collaborator

Kanaye commented Dec 5, 2016

I'm currently working on this and stumbled accross a hard-dependencie in the DataSource.js (or precisely here ).
We could:

  1. reimplement the missing methods: size, at, isEmpty and each
  2. make them a 'soft' dependencie so they would only exist when jsvalue is included once it's a separate lib (I plan on doing some integration so that value and blocks would not override them when they are both included in the same document)
  3. remove them completely.

What do you think ?

@astoilkov
Copy link
Owner Author

I think we can remove them. They don't enough value to reimplement them and I don't like optionally adding methods.

@Kanaye
Copy link
Collaborator

Kanaye commented Dec 13, 2016

I created a branch that's resolving this.
I mostly removed internal usage of jsvalue apis and just moved a few over to jscore.
Namely: blocks.toObject, blocks.flatten and blocks.pairs.

We are loosing a lot of functionality and comfort with this (e.g. no more collection.each()) but I that should be expected.

I didn't removed the other optional "integrations" (e.g. in observable.extend()) as they are documented to be only working with jsvalue and will come handy once jsvalue is a seperate lib.

All tests are passing and I tested server- and client-side rendering with a modified version of the shopping example.

In my opinion we should release at least one more 0.3.x version with jsvalue included and than merge this. (I'll rebase it then so we have a clean historie).


Edit:
I pushed the mentioned version of the shopping example in a separate branch over here.
It doesn't looks as nice as the version on the master because of the removal of the utility methods.
I didn't want to drop in some other library like lodash because it wasn#t that much and I don't like to use other libs in examples.

@astoilkov
Copy link
Owner Author

Ok. Everything sounds great.

Regarding the version release: I agree. Do you know how to do this or should I do it?

@Kanaye
Copy link
Collaborator

Kanaye commented Dec 15, 2016

I can look into building a release.
I'm also planning to (or at least I'm think about) make travis build and publish releases (tagged commits on the master branch).
At least if you agree that it could be usefull.
But maybe I'm just a to big fan of automating stuff.

I'm currently working on a fix for #126 and I think I can fix it within the next week.
So I would wait with the next release until that's done.

@astoilkov
Copy link
Owner Author

Nice. I am a big fan of automating stuff too. However, can you describe a little more the idea you have about travis. It seems I am not familiar with it.

@Kanaye
Copy link
Collaborator

Kanaye commented Dec 17, 2016

I would let travis push the build folder to npm, bower and github (and probably some other destination I missed) via an deployment script hook with the options tags set to true.
I would store the API-Keys, passwords, etc. in the env via the Repository Settings this ensures that the keys aren't leaked.
These env var's are also only provided to trusted builds. So builds triggered by the repository itself and not by pull-request for example.

I will probably write the deploy script as some node script because I really don't like bash scripts (my only bash scripts are for setting up my own dev-env e.g. when I have to reinstall my computer).

@astoilkov
Copy link
Owner Author

Now I understand. This sounds really useful and time saving. Great idea.

I haven't done such thing before so I am excited to see it implemented.

@Kanaye
Copy link
Collaborator

Kanaye commented Jan 25, 2017

Hmm okay after thinking and research for a while I've found one major problem I wasn't aware of: bower.
Or to be more precise: the fact that bower uses tags (and the commit bound to the tag) for their version managment.
That means with the workflow explained above won't work because bower won't have the build files within the tagged commit.
In theory we could mess with the tag like delete it and then recreate it with the new commit .. but ehh.. no, that causes trouble with like all the things regarding git and remotes.

My new approach would be:
Watching in the travis script for a change in the version property of the root package.json on the master branch and if it changed rebuild jsblocks, create a commit with the build files and a tag with the version and upload the files to the github release and maybe others.
Only downside (at least the only one I came up with now): We have one 'unecessary' commit.
A commit log could look that-ish:

Author: Joscha Rohmann ...
Date: ...
    Bump version to x.x.x

Author: Blocks CI ...
Date ...
    Build Release x.x.x

This would require: A new Github User (I would suggest Block CI or something like that) and fiddling with git itself (I'll propbably use node-git for that).

What Do you think, if you agree you can decide if you yourself or I should create a new github user for that.
That user would require push-access to this repository (I'll test on some other repo) and I just would need the api-key and username and password (you could add them to the tavis env as linked above under GITHUB_USER, GITHUB_PASSWORD and GITHUB_KEY if you decide to create the user yourself.)


another offtopic:
I would also like to let travis run the tests on sauce labs (more automated real browser testing and a nice badge ;) ).
sauce labs has a free for open source projects plan: open sauce.
They also have a plugin for karma so it should be fairly easy to set up travis to run our tests their.
As above if you agree I can create an account or you can create it and invite me (the address linked in my github profile will be fine for that ;) )

@astoilkov
Copy link
Owner Author

astoilkov commented Jan 31, 2017

@Kanaye Sorry for getting late with the response. I have a lot of work these few days. I will take a look at everything soon and let you know. On first glance I think you can create the account.

@Kanaye
Copy link
Collaborator

Kanaye commented Jan 31, 2017

No stress. I know that situation well. Take your time

@astoilkov
Copy link
Owner Author

Ok. I would say that if bower is causing too much trouble we could stop support for it. It seems bower is not becoming more popular even the opposite. If you have a solution which is much easier we can easily drop support for bower.

However, if you think we should continue its support I agree with your approach and I don't see any problem with the additional commit. You could create the account yourself.

Regarding Sauce Labs: I really like the idea. If it is easy to setup this would be a great enhancement. Again you could create the account and just invite me in.


offtopic:
Did you take a look at BrowserStack? I always wondered which one of them is better. Do you have any observations?

@Kanaye
Copy link
Collaborator

Kanaye commented Feb 7, 2017

We can still support bower. I just need to get a diff of the package.json on the latest master commit and test if the version changed. That's a problem I already solved. I'm using nodegit to access the repository so that's not that complicated.

I wanted to build a nodejs-ish styled commit-, issue- and pr log that are included (means commited, closed and merged) from the last release to the current. I thought that's a nice addition for developers to see what's changed and fixed. But unforunatly I stumbled accross a bug in nodegit (or libgit2) (I'm currently talking to both maintainers to solve this) and the current github-api doesn't contain the information I'd need without hitting the api-call-limits :/
The current beta/GraphQL github API looks like it might solves that problem.
Otherwise I'll skip that for now and wait for the nodegit/libgit2 bug to be fixed.

Regarding Sauce Labs/Browser Stack:
The company I'm working for has accounts for both services,
Sauce Labs for automated testing and Browser Stack for manual. (Don't ask me why they have both. I don't understand it).

Sauce Labs sometimes has browsers disconnecting or hanging in automated testing (but could also be an issue with my companies network connection). That results in false failing tests.

With the exception of sometimes a a little bit laggy/slow connection to browser stack I didn't had any trouble in past. But I haven't used them with automated tests yet.

We can create accounts for both services and test which one is better/more stable/etc. and can than decide which one we prefer.

@Kanaye
Copy link
Collaborator

Kanaye commented Feb 7, 2017

Okay after playing a bit with the github GraphQL API it looks promising.
But I'm not sure it'll do all the things I want it todo.
I haven't used GraphQL APIs for anything right now so I have to look into this a bit deeper.
Probably within this week, once I'm done I'll open a PR to master with the publish task and a checklist of what is done to make sure I haven't missed anything.
I'll also create the accounts until then.

@astoilkov
Copy link
Owner Author

Nice. I read about GraphQL and it looks very promissing. It would be awesome to see some real world usage of it.

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

No branches or pull requests

2 participants