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

[FEATURE] Replace dyfactor with puppeteer #118

Merged
merged 10 commits into from
Jun 18, 2019

Conversation

pzuraq
Copy link
Collaborator

@pzuraq pzuraq commented Jun 18, 2019

This PR updates the codemod in the following ways:

  1. Enables decorators by default, now that they are a first class API in Ember
  2. Switches to using Puppeteer directly for gathering telemetry data, instead of using dyfactor. This allows us to provide a much lighter codemod in a single package, rather than requiring users to install the dyfactor plugin.
  3. Moves to storing the telemetry data in the OS tmpdir instead of directly in the repo, so users won't accidentally check it in.
  4. Changes the script to require/assume telemetry data always exists, and bail out if it can't be found for a given class. This part may be a bit aggressive - alternatively, we could just attempt to codemod the value even if we can't find the telemetry data for it.
  5. Adds a (very) rudimentary integration test, which actually runs the full codemod against an Ember app to validate that everything is working.

It also updates the command users use to run the codemod. They no longer have to provide the name of the codemod as the first argument, and should instead provide the URL to gather telemetry at:

# before
ember-es6-class-codemod ember-object ./app

# after
ember-es6-class-codemod http://localhost:4200/ ./app

@pzuraq pzuraq force-pushed the replace-dyfactor-with-puppeteer branch from 4bb1a49 to f0c5074 Compare June 18, 2019 05:10
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Looking really great, thanks for working on this!

Can you update the top level package.json (the codemod's own) to drop Node 6 and 11 support from the engines field? I noticed we changed CI to use Node 8, so we should make sure to update engines and "bump majors" to 0.2.0.

lib/gather-telemetry.js Outdated Show resolved Hide resolved
@@ -0,0 +1,20 @@
# EditorConfig helps developers define and maintain consistent
Copy link
Member

Choose a reason for hiding this comment

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

Thoughts on adding ember-cli as a devDep and generating this dynamically? The vast majority of these files do not need to be in the repo, we could generate a new app and then only talk about the "diff" between what we added.

Anyways, I think this is a good follow up PR, we should not block landing on this...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking about this, and would like to. My main concerns are properly generating the output application, and the ability to add arbitrary test scenarios in the integration app. I'd like to write more complex scenarios that involve telemetry out in the integration test. So, we'd need some way to generate most of the app, and then copy the app folder over.

Probably we could just do that, just check in the app folders and generate/compare them. I'll work on that.

Copy link
Member

Choose a reason for hiding this comment

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

@pzuraq - Let's make an issue to track that, I'd like to circle back around and this is good enough for now...

transforms/helpers/util/get-telemetry-for.js Outdated Show resolved Hide resolved
transforms/helpers/util/get-telemetry-for.js Show resolved Hide resolved
@rwjblue
Copy link
Member

rwjblue commented Jun 18, 2019

Updated to address feedback. Should be "good to go"...

@rwjblue rwjblue added breaking enhancement New feature or request labels Jun 18, 2019
@rwjblue rwjblue merged commit 4de99d2 into master Jun 18, 2019
@rwjblue rwjblue deleted the replace-dyfactor-with-puppeteer branch June 18, 2019 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants