Skip to content

Fixes datasource generation with community connector #180

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

Closed
wants to merge 1 commit into from

Conversation

amuramoto
Copy link

Community connectors are not listed in availableConnectors, causing the following when a user chooses 'other' and enters a community connector:

ERROR Cannot read property 'package' of undefined

Fix makes function return when this is the case, allowing generator to proceed creating skeleton record in datasources.json

Community connectors are not listed in availableConnectors, causing the following when a user chooses 'other' and enters a community connector:

ERROR Cannot read property 'package' of undefined

Fix makes function return when this is the case, allowing generator to proceed creating skeleton record in datasources.json
@slnode
Copy link

slnode commented May 11, 2016

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@superkhau
Copy link
Contributor

@amuramoto Your PR is failing on Travis, can you check it out?

@superkhau superkhau self-assigned this May 11, 2016
@amuramoto
Copy link
Author

@superkhau Tests appear to be failing due to PR #173 which is still under review

@superkhau
Copy link
Contributor

superkhau commented May 11, 2016

@amuramoto Cool, I'll leave this one for @bajtos to review as he is more familiar with this module. It LGTM, but can't really tell with Travis not passing.

@superkhau
Copy link
Contributor

@amuramoto Can you add a unit test to verify your changes?

@@ -198,6 +198,7 @@ module.exports = yeoman.generators.Base.extend({

installConnector: function() {
var connector = this.availableConnectors[this.connector];
if (!connector) return;
Copy link
Member

Choose a reason for hiding this comment

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

When we don't have any information about the custom connector selected for a new datasource, then I think we should assume the package name is same as the connector name and run npm install {this.connector}.

@amuramoto @superkhau @ritch @raymondfeng Thoughts?

@bajtos
Copy link
Member

bajtos commented May 18, 2016

@amuramoto thank you for the pull request, this is indeed a bug that needs to be fixed!

I left a comment above, I think we should take a different approach. Also please add a unit-test to verify your change and prevent regressions in the future, see these two existing tests for inspiration:

it('allow connector without settings', function(done) {
var modelGen = givenDataSourceGenerator();
helpers.mockPrompt(modelGen, {
name: 'kafka1',
customConnector: '', // temporary workaround for
// https://github.com/yeoman/generator/issues/600
connector: 'kafka',
installConnector: false
});
var builtinSources = Object.keys(readDataSourcesJsonSync('server'));
modelGen.run(function() {
var newSources = Object.keys(readDataSourcesJsonSync('server'));
var expectedSources = builtinSources.concat(['kafka1']);
expect(newSources).to.have.members(expectedSources);
done();
});
});
it('should install connector module on demand', function(done) {
var modelGen = givenDataSourceGenerator();
helpers.mockPrompt(modelGen, {
name: 'rest0',
customConnector: '', // temporary workaround for
// https://github.com/yeoman/generator/issues/600
connector: 'rest'
});
modelGen.run(function() {
var pkg = fs.readFileSync(
path.join(SANDBOX, 'package.json'), 'UTF-8');
pkg = JSON.parse(pkg);
/* jshint -W030 */
expect(pkg.dependencies['loopback-connector-rest']).to.exist;
done();
});
});

For posterity, the failing CI build is not waiting for #173, but for fixing our dependencies, per
#173 (comment)

Yeoman's latest release has a breaking change in prompts. We probably have to update our code.
yeoman/generator#926 The latest version of yeoman-environment 1.6.1 is breaking the unit test. The promptModule is causing problems.

@bajtos bajtos added the bug label May 18, 2016
@bajtos
Copy link
Member

bajtos commented Jun 13, 2016

Closing in favour of #199

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

Successfully merging this pull request may close these issues.

5 participants