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

Should we be using .remote()? #9

Closed
sayedihashimi opened this issue Sep 9, 2014 · 10 comments
Closed

Should we be using .remote()? #9

sayedihashimi opened this issue Sep 9, 2014 · 10 comments
Labels

Comments

@sayedihashimi
Copy link
Member

@shirhatti in the current implementation we are using .remote() in index.js to retrieve the template content. What I've noticed is that when calling .remote() if the git repo exists on the client cache then it will not be refreshed. There is a refresh parameter on remote(). If we specify true for refresh then the git repo will be cloned each time when the generator is invoked. On my box in order to get rid of yeoman cache I had to remove yo.

Is using .remote() the right approach here? I looked at the generator-angular and they include the files directly inside the npm package then use .template() and .copy() to write the files.

The code to always update when using remote is below.

    retrieveContent: function () {
        var done = this.async();

        this.remote('ligershark', 'aspnet_vnext_samples', 'master', function(err, remote) {
            done();
        },true);
    }

cc @jchannon

@jchannon
Copy link
Contributor

jchannon commented Sep 9, 2014

I've spotted the same behaviour having to delete the hidden yeoman cache
folder. Remote makes sense if it will clone every time or as you say we
include the templates. One thing keeping it remotely is that you can change
the templates without having to do a new release on npm for the generator.

Let me know what is decided as the nancy generator does the same :)

@shirhatti
Copy link
Member

I agree with @jchannon in that keeping the examples distinct allows us to update the samples without updating the generator. This is especially useful since the K runtime is not yet stable and has breaking API changes.

Additionally, using .remote() allows to include project templates types discussed as in ligershark/Kulture#7

@sayedihashimi
Copy link
Member Author

@shirhatti @jchannon thanks for the comments.

What I'm hearing is that we chose to use .remote() because it can get the latest content every time. What I don't like about that is that it will clone the entire repo every time. So to create a single project (let's say a Console project) it will clone a repo that has console/web/mvc into the cache folder.

Instead I think it would be better to have the content directly embedded in the npm package. Updating the npm package is pretty easy, and gives people an opportunity to pick the specific version they are interested in using.

For now let's just change the code to add true for Refresh on the call to .remote(). Later we can investigate if we want to move the files into the npm package itself.

@sayedihashimi
Copy link
Member Author

I have just made the update to pass true to .remote()

@sayedihashimi
Copy link
Member Author

Another drawback of using .remote, it makes it more difficult to test locally. From what I can tell the best we can do is push to a remote branch and update index.js to point to that other branch for local testing. With a single contained npm package you can use npm link for local testing.

@spboyer
Copy link

spboyer commented Dec 2, 2014

using .remote() makes it somewhat challenging when contributing. for example adding an additional project type means submitting 2 PRs; one to the generator and on to the vnext_examples OR hosting the new example.

I would like to see it similar to the angular generator.

@sayedihashimi
Copy link
Member Author

@spboyer I agree let's get rid of using .remote its a pain to deal with as you stated. And let's combine aspnet-item with generator-aspnet.

In addition to this, I discussed with @jchannon and @nosami and we don't think that the generators should be under LigerShark. Instead we should move them under OmniSharp. Currently we have the following repositories that are related which should be combined.

  1. generator-aspnet
  2. aspnet-vnext-samples
  3. nancy generators samples
  4. aspnet-item

I think we should simplify this and bring it down to a single repository on OmniSharp and get rid of using .remote().

I think the correct order to execute this is as follows.

  1. Create new repo in OmniSharp and move generator-aspnet as well as the samples (including Nancy samples)
  2. Update index.js to not use .remote()
  3. Release npm package with that content and test it out to ensure no issues
  4. Add aspnet-item to the generators package
  5. Release updated npm package with aspnet-item

Thoughts?

I'm on vacation until Sunday so I can only do minimal work until then. I can get the new repo in OmniSharp created and I can add you as a contributor.

FYI @kenwarner seemed like was very interested in helping with generators in the past (haven't heard much recently though) so maybe he can help as well?

@sayedihashimi
Copy link
Member Author

OK I have created the new repo at https://github.com/OmniSharp/generator-aspnet/. I've moved the sample files under samples including the nancy ones from @jchannon.

I've created an issue to update index.js to pull from this new repo OmniSharp/generator-aspnet#1.

@sayedihashimi
Copy link
Member Author

@spboyer I have given you push access to https://github.com/OmniSharp/generator-aspnet/.

@sayedihashimi
Copy link
Member Author

Migrated to OmniSharp/generator-aspnet#26

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

No branches or pull requests

4 participants