Skip to content

Conversation

evenstensberg
Copy link

@evenstensberg evenstensberg commented Apr 4, 2017

Adds a env.getArgument function to env, as there's no way of getting an this object inside a generator. If you want to run a feature based on an internal object of the generator, this opens up the opportunity for that.

I found no "real" effective way to get calls from the child class, so I've made a copy of the this object at initialization, where we later compare the initial state of the this object to the updated this after it has been initialized. This way we can get the added this objects from an generator.

Examples:

Generator

module.exports = class WebpackGenerator extends Generator {
	constructor(args, opts) {
		super(args, opts);
		this.configuration = {}
	}
	prompting() {
		return this.prompt([Input('entry','What is the name of the entry point in your application?'),
			Input('output','What is the name of the output directory in your application?')]).then( (h) => {
					this.configuration = h;
			});
	}
};

Yeoman Env

env.run('npm:app')
  .on('end', () => {
        const myArg = env.getArgument('configuration');
       // logs { entry: ..., output: ...} 
  });

Copy link
Author

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want this changed?

lib/index.js Outdated
@@ -373,6 +374,18 @@ class Generator extends EventEmitter {
this._running = true;
this.emit('run');

const restArgs = Object.keys(Object.assign({}, this)).filter(orginalArg => {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this?

@addyosmani
Copy link
Member

addyosmani commented Apr 4, 2017

I found no "real" effective way to get calls from the child class, so I've made a copy of the this object at initialization, where we later compare the initial state of the this object to the updated this after it has been initialized.

In general, I'm fine with this approach and want to ensure the generators API is flexible enough to support Webpack CLI's needs here. I would say that once we've had a look from @SBoudrias @sindresorhus or @silvenon it would be good to get some tests added for this addition.

Update: On second read through, it looks like the way we're exposing this feature might limit the ability to work with anything other than the last generator that came through.

@evenstensberg
Copy link
Author

evenstensberg commented Apr 4, 2017

Perhaps a bit more consistent way, is to check if this.config has been declared in the constructor of a generator, and we do not write a .yo-rc.json file if it is. I find it really inefficient to do a write and read based on a object I rather can pull out of the generator itself. Tho this is good(?), this PR allows any object declared in a constructor to be pulled out, so it allows more freedom on the authors side of this.

Edit: Do you want me to rewrite the object names to something more indicative?

(orginalArg.substring(0, 1) !== '_') && (orginalArg !== 'orginalArgs');
});

this.env.getArgument = function (name) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line here is problematic as this environment object is shared between all generator instances. With this code, only the last ran generator will be available through getArguments.

Maybe a way to access a generator instance would be more flexible?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be a good match. Any ideas on how to achieve that on the programming end?

@SBoudrias
Copy link
Member

Maybe you'd want to explore instantiating the generator on your own and reading from the instance; kind of like Environment#run works https://github.com/yeoman/environment/blob/master/lib/environment.js#L412

var generator = env.create('webpack', { args, options });
generator.run(() => {
  console.log(generator.configuration);
});

@evenstensberg
Copy link
Author

Would be great, but we're dependant on env, as we're doing composeWith for users, if they supply with multiple arguments in --init as well as we have our own adapter for all the generators being initiated. Open to suggestions!

@evenstensberg
Copy link
Author

In hindsight, I think the best option here, is to make us get this.config from a generator instead of doing a write. Seems approachable if yeoman can update the config without doing a read/write

@SBoudrias
Copy link
Member

@ev1stensberg Hey, went over the webpack-cli code yesterday. There's not much to see yet, but I'm really unclear about what you guys are trying to achieve. It looks to me like you're only using Yeoman for the prompt system. I think you told me that was a temporary step in your timeline, but it's hard to see what's the vision and the end goal here.

Let me know if some concept of Yeoman seems foreign to you. Maybe a better theoretical overview would help you put the pieces together?

From a technical perspective, Yeoman does not read/write each time someone edit the config. Instead, we save all the file system state into an in-memory file system (backed by mem-fs and located on the Environment#sharedFs property). Each file change will triggers a change event on the mem-fs instance. Here's how we use that inside yeoman-generator to make sure there's always a file write happening at the end of the queue. In theory, you could listen for that event to retrieve the content of any config files - and you could delete that file from the mem-fs store so it won't be written to disk.

That solution does seems a bit complicated to me. I wonder if you should rather create a base generator-webpack. To extend the behavior, other generators would just have to compose with the base one and pass the options it wishes to overwrite.

@evenstensberg
Copy link
Author

@SBoudrias Hey! Thanks for having a look. Indeed, this is a temp hack. Might be alright to run from a fork after all. Shortly after we've released, we're going to go with what you mentioned.

Think I've got a good overview of Yeoman, so there's no real need of that. Here's the source code with the solution implemented, but we have to go to env.opts.env for each generator in order to get this.configuration.

I really want to make use of this.config instead once we've moved this to an yeoman generator.

Here's how we use that inside yeoman-generator to make sure there's always a file write happening at the end of the queue. In theory, you could listen for that event to retrieve the content of any config files - and you could delete that file from the mem-fs store so it won't be written to disk.

Is there any way to discard the file save and read from an variable instead for this?

@evenstensberg
Copy link
Author

evenstensberg commented Apr 27, 2017

@SBoudrias Asking if you could make a change to this.config to not save, compared to using physical write for a save in the current version. We can have an option whereas if you have the option supplied somewhere within your generator, you won't do a fs.writeFile for this.config, but rather provide the object as it is, in the generator. This would be great for us.

Going to close this, as we're going to transition into using yeoman generators explicitly pretty soon. For now, running against a local fork is fine, but I'm a bit nervous about how the this.composeWith will turn out instead of env.configuration, when we run the webpack-cli generator in near future.

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

Successfully merging this pull request may close these issues.

3 participants