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

fix: conditionally require lint, format #49

Merged
merged 6 commits into from
Aug 31, 2017
Merged

fix: conditionally require lint, format #49

merged 6 commits into from
Aug 31, 2017

Conversation

kjin
Copy link
Contributor

@kjin kjin commented Aug 18, 2017

Addresses #48

@kjin kjin requested a review from ofrobots August 18, 2017 22:59
@ofrobots
Copy link
Contributor

@JustinBeckwith WDYT?

If we are doing this, then we should also add a compile verb, and stop requiring the user to depend on typescript. Otherwise the user may have a different version of typescript than the one we use and may see spurious errors with one but not the other. Is there any reason other than the compile verb for the user to depend on typescript?

This would imply that typescript version is determined by gts rather than the user – who get locked into the version we use.

Another way to fix this would be to make sure we never have import typescript on the path reachable from gts init. @kjin do you this this is feasible?

@JustinBeckwith
Copy link
Collaborator

I appreciate what you're trying to do - but I think this introduces more problems than it solves. It's so very important to let the user control the version of tsc they use with their project. That's truly going to be different for every user.

Is there a way to have init fail if typescript isn't already installed? Or install typescript as a peer dependency for the user?

@kjin kjin changed the title fix: set typescript as dependency fix: conditionally require lint, format Aug 30, 2017
@kjin
Copy link
Contributor Author

kjin commented Aug 30, 2017

@ofrobots @JustinBeckwith PTAL

src/cli.ts Outdated
@@ -74,9 +75,13 @@ async function run(verb: string): Promise<boolean> {
yes: cli.flags.yes || cli.flags.y || false,
logger: logger
};
// See: https://github.com/google/ts-style/issues/48
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it would be better to explain the reason why we are doing this here in a comment. You can additionally link to the issue, but the gist of the intention should be clear here.

@ofrobots
Copy link
Contributor

For posterity, it is worth documenting why we don't want to directly depend on typescript (i.e. enforce the typescript version for the users of gts):

  • TypeScript doesn't follow semver. This means that a minor update to the typescript version could result in compile errors in users' code. Directly depending on typescript would mean breaking changes for gts users whenerver we bump the typescript dependency.
  • Users may want newer features so they may want the latest version of TypeScript. Each user should be able to move at their own pace on when they want to adopt the new features. They need to be at the minimum enforced by the peerDependency requirement we have, but if they want to be newer, that is fine.

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.

4 participants