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

feat(schematics): add migration schematics for schematics users #3538

Merged
merged 7 commits into from Apr 11, 2018

Conversation

Projects
None yet
5 participants
@benlesh
Copy link
Member

benlesh commented Apr 9, 2018

No description provided.

@benlesh benlesh requested a review from jasonaden Apr 9, 2018

@hansl
Copy link
Contributor

hansl left a comment

I'm blocking this as I actually test it manually locally.

const pkgPath = '/package.json';
const buffer = tree.read(pkgPath);
if (buffer == null) {
throw new SchematicsException('Could not read package.json');

This comment has been minimized.

@hansl

hansl Apr 9, 2018

Contributor

This is a runtime dependency. Just throwing Error is fine though.


function runTsLint() {
return (tree: Tree, context: SchematicContext) => {
const workspace = getWorkspace(tree);

This comment has been minimized.

@hansl

hansl Apr 9, 2018

Contributor

This limits the update schematic to angular CLI projects. I'm pretty sure you don't need this restriction; you should be able to run tslint over the root tsconfig (or all TS files). We have a Tslint task now.

This comment has been minimized.

@benlesh

benlesh Apr 9, 2018

Member

Fair... runTsLint removed anyhow, as it's probably better to have them run tslint fix manually as a second step (so it's easier to see where breakage occurs)

@Brocco
Copy link
Contributor

Brocco left a comment

Looks like a file is included that is not needed.

@@ -0,0 +1,5 @@
export const latestVersions = {

This comment has been minimized.

@Brocco

Brocco Apr 9, 2018

Contributor

This file does not seem to be used anywhere

@IgorMinar
Copy link
Collaborator

IgorMinar left a comment

The rest looks good to me.

}

pkg.dependencies['rxjs-compat'] = rxjsCompatVersion;
pkg.devDependencies['rxjs-tslint'] = rxjsTSLintVersion;

This comment has been minimized.

@IgorMinar

IgorMinar Apr 10, 2018

Collaborator

Can we skip this install for now? Since we 3ant people to primarily update their app to v6 we shouldn't bring deps they absolutely don't need. Because if we do they will not know when it's ok remove them.

@IgorMinar

This comment has been minimized.

Copy link
Collaborator

IgorMinar commented Apr 10, 2018

Thinking about this more, I believe that the best solution would be to move the contents of rxjs-lint into this package. That way, nobody has an extra package to manage.

I could be convinced of keeping it as a separate package if there significant complications in merging the tslint transforms into the main package, but in that case we should consider renaming the package to something else than "-lint" because that name is confusing. Maybe rxjs-v6-upgrader (like what @angular/material does with "angular-material-v6-updater")

cc: @mgechev

@benlesh

This comment has been minimized.

Copy link
Member

benlesh commented Apr 10, 2018

@IgorMinar my only concern is that publishing the packages in this repository is already pretty complicated, and worse, it's still manual. I'm afraid I'm too dumb to complicate it anymore than it already is.

If publishing were more automated, then I'd completely agree. Right now I only mostly agree. lol

@mgechev

This comment has been minimized.

Copy link

mgechev commented Apr 10, 2018

I can see all the pros of moving the migration rules to the rxjs package and I agree it sounds ideal. If this is going to complicate the publishing process, I think there's one more option which keeps the changes here to the minimum.

We can keep all TSLint migration and linting rules for RxJS separately (as rxjs-tslint is right now), and introduce a TSLint configuration preset for migration from version 5 to 6 as part of the rxjs package. This will look something like:

{
  "rules": {
    "update-rxjs-imports": true,
    "migrate-to-pipeable-operators": true,
    "collapse-rxjs-imports": true
  }
}

The pros of this solution that I see are:

  • We keep the changes to the rxjs package & repository to the minimum
  • RxJS 6 knows about the migration process from RxJS 5 (the tslint rule preset)

As for cons:

  • The maintenance of two packages is still necessary
@hansl

This comment has been minimized.

Copy link
Contributor

hansl commented Apr 10, 2018

@benlesh @IgorMinar How about make the linting rules part of the rxjs-compat package?

@IgorMinar

This comment has been minimized.

Copy link
Collaborator

IgorMinar commented Apr 10, 2018

@hansl

hansl approved these changes Apr 10, 2018

@benlesh benlesh force-pushed the benlesh:schematics branch from 59df616 to 6a2c6ce Apr 11, 2018

@benlesh benlesh merged commit 78d86a1 into ReactiveX:master Apr 11, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lock

This comment has been minimized.

Copy link

lock bot commented Jun 5, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 5, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.