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

Save Generator Version in Environment #90

Closed

Conversation

andersonkyle
Copy link

The first step of yeoman/yo#542 involves persisting the generator version # when the Environment is being initialized. That way we'll have access to it later when we want to display it to the User.

@mischah
Copy link
Member

mischah commented Oct 19, 2017

Thanks for the PR :octocat:

@mischah
Copy link
Member

mischah commented Oct 19, 2017

@SBoudrias Do you have an idea why the builds are failing lately?

Update: Test are passing locally in master branch.
Update: I updated travis to run test on Node 8 additionally. 1 test is failing. See #91

lib/resolver.js Outdated
for (const modulePath of generatorsModules) {
patterns.push(path.join(modulePath, lookup));
for (const module of generatorsModules) {
patterns.push({...module, path: path.join(module.path, lookup)});
Copy link
Member

Choose a reason for hiding this comment

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

I guess we are not ready to use object rest/spread properties 😞

lib/resolver.js Outdated
for (const filename of globby.sync('*/index.js', {cwd: pattern})) {
this._tryRegistering(path.join(pattern, filename));
for (const filename of globby.sync('*/index.js', {cwd: pattern.path})) {
this._tryRegistering({...pattern, path: path.join(pattern.path, filename)});
Copy link
Member

Choose a reason for hiding this comment

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

I guess we are not ready to use object rest/spread properties 😞

@andersonkyle andersonkyle force-pushed the save-generator-version-in-env branch from 20e9dd4 to d080adb Compare October 20, 2017 14:47
@andersonkyle
Copy link
Author

Removed the use of ...spread operators.

@andersonkyle andersonkyle force-pushed the save-generator-version-in-env branch from d080adb to b20a432 Compare October 20, 2017 16:26
@Awk34
Copy link

Awk34 commented Oct 20, 2017

Code LGTM

lib/store.js Outdated
this._meta[namespace] = {
resolved: path,
namespace
namespace,
version
Copy link
Member

Choose a reason for hiding this comment

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

Reading/requiring files is a pretty costly operation. I wonder if instead we wouldn't be better to setup version as a property getter that will lazily require the package.json at the resolved location when the property is accessed.

Then we wouldn't incur the performance hit for every single operations, only when we'll actually display the version number.

When storing as module instead of path, then we could just setup the version as unknown.

@andersonkyle andersonkyle force-pushed the save-generator-version-in-env branch from b20a432 to af55d79 Compare October 24, 2017 21:40
@andersonkyle andersonkyle force-pushed the save-generator-version-in-env branch from af55d79 to d607f3b Compare October 24, 2017 22:47
@andersonkyle
Copy link
Author

@SBoudrias Switched to a getter for the version but it is still a WIP. A few failing tests still need to be resolved.

@SBoudrias SBoudrias force-pushed the master branch 2 times, most recently from 33d5f59 to 3393b40 Compare August 4, 2018 15:58
@LitoMore
Copy link

Any update?

@SBoudrias SBoudrias closed this Oct 27, 2019
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.

5 participants