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

Adds a Polymer 3 element template to init. #115

Merged
merged 13 commits into from
Apr 13, 2018
Merged

Conversation

bicknellr
Copy link
Member

No description provided.

@bicknellr bicknellr requested review from aomarks and rictic April 12, 2018 23:41
@bicknellr
Copy link
Member Author

This still doesn't install.

Copy link
Contributor

@rictic rictic left a comment

Choose a reason for hiding this comment

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

Nice!

It should have a test IMO. Let's add a test of this template in test/integration/integration_test

@bicknellr
Copy link
Member Author

bicknellr commented Apr 13, 2018

I added an integration test, there were a handful of problems though:

// compiled out?
async prompting() {
return super.prompting();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you double check that this doesn't work when this isn't here? I looked at the JS that's produced, and we're producing ES6 here. I can't see any reason why this code would do anything at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems like async-await is being compiled out. I'm betting it has something to do with that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, yes, the parent class' prompting function isn't called if this isn't here. I can't figure out where yeoman-generator calls it though and I'm not sure how it gets called due to the async-await thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, telling TypeScript to target ES2017 still has this problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here's the problem: https://github.com/yeoman/generator/blob/v1.0.1/lib/index.js#L382
Yeoman only checks the object's prototype's own properties, so I think I have to have something like this in here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added pass-through functions for all of the other methods as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh hey, you found it too. I just filed yeoman/generator#1065

// packages.
await exec('npm install', {cwd: dir});

// TODO(bicknellr): Add this back in when `polymer lint` has a Polymer 3
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use test urls instead of usernames for TODOs. #130 for this one

// await runCommand(binPath, ['lint'], {cwd: dir});

// TODO(bicknellr): Remove the `--module-resolution=node` argument once
// `polymer test` passes them in correctly
Copy link
Contributor

Choose a reason for hiding this comment

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

await runGenerator(createElementGenerator('polymer-3.x'))
.withPrompts({name: 'my-element'}) // Mock the prompt answers
.toPromise();
// TODO(bicknellr): Use `polymer install` once it supports installing npm
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@rictic rictic left a comment

Choose a reason for hiding this comment

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

Nice!

@@ -121,8 +121,14 @@ export function createElementGenerator(templateName: string):
};

class Polymer3ElementGenerator extends ElementGenerator {
// TODO(bicknellr): Why doesn't this inherit properly? Because `async` is
// compiled out?
// This is not a no-op: Yeoman only checks the object's prototype's own
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a mention of yeoman/generator#1065 here with these not a no-op comments.

@bicknellr
Copy link
Member Author

One of the tests timed out; I'm going to restart that Travis build.

@bicknellr bicknellr merged commit 4349dbe into master Apr 13, 2018
@bicknellr bicknellr deleted the init-3.x-element branch April 13, 2018 19:18
abdonrd pushed a commit to abdonrd/tools that referenced this pull request Apr 17, 2018
* Update dependencies.

* Update vscode-languageserver as well.

* More type safe ClientCapabilities.

* Standardize import style of vscode-uri
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.

3 participants