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

Add warning for TypeScript set up issues and fix Babel config #10998

Merged
merged 2 commits into from
Jun 2, 2020

Conversation

mrmckeb
Copy link
Member

@mrmckeb mrmckeb commented Jun 1, 2020

Issue: #10943

What I did

  • Added a warning for projects that have TypeScript files, but not TypeScript as a dependency.
  • Addressed an issue where optional chaining and nullish coalescing may not be processed correctly.

How to test

Tests have been added for the CLI.

@mrmckeb mrmckeb force-pushed the mrmckeb/issue10943 branch 2 times, most recently from f0b4d41 to 34bc671 Compare June 1, 2020 10:02
@mrmckeb mrmckeb changed the title Mrmckeb/issue10943 Add warning for TypeScript set up issues and fix Babel config Jun 1, 2020
@mrmckeb mrmckeb force-pushed the mrmckeb/issue10943 branch from 34bc671 to c859205 Compare June 1, 2020 15:17
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

One comment otherwise LGTM


export const warn = ({ hasTSDependency }: Options) => {
if (!hasTSDependency) {
const hasTSFiles = !!globby.sync(['**/*.(ts|tsx)', '!**/node_modules', '!**/*.d.ts']).length;
Copy link
Member

Choose a reason for hiding this comment

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

'**/*.(ts|tsx)' is not a proper glob 😄 See #10926

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

According to the docs here:
https://github.com/isaacs/minimatch#usage
https://github.com/sindresorhus/globby#patterns

This is supported, and is called extglob. Maybe you can give me some more info around the change? I didn't see any docs linked in the PR referenced.

Copy link
Member

Choose a reason for hiding this comment

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

const hasTSFiles = !!globby.sync(['**/*.(ts|tsx)', '!**/node_modules', '!**/*.d.ts']).length;

should be

const hasTSFiles = !!globby.sync(['**/*.@(ts|tsx)', '!**/node_modules', '!**/*.d.ts']).length;


export const warn = ({ hasTSDependency }: Options) => {
if (!hasTSDependency) {
const hasTSFiles = !!globby.sync(['**/*.(ts|tsx)', '!**/node_modules', '!**/*.d.ts']).length;
Copy link
Member

Choose a reason for hiding this comment

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

const hasTSFiles = !!globby.sync(['**/*.(ts|tsx)', '!**/node_modules', '!**/*.d.ts']).length;

should be

const hasTSFiles = !!globby.sync(['**/*.@(ts|tsx)', '!**/node_modules', '!**/*.d.ts']).length;

@mrmckeb mrmckeb force-pushed the mrmckeb/issue10943 branch from c859205 to 34506bd Compare June 2, 2020 08:16
@mrmckeb
Copy link
Member Author

mrmckeb commented Jun 2, 2020

OK, I found the reference here:
https://www.npmjs.com/package/extglob#extglob-cheatsheet

@@ -0,0 +1,39 @@
import globby from 'globby';
Copy link
Member

Choose a reason for hiding this comment

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

any reason to add a dep when we already depend on glob?

Copy link
Member Author

Choose a reason for hiding this comment

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

In CLI, there was no dependency. Globby seemed more reliable from my (quick) testing of the two, and supports arrays. This implementation was lifted from the same check in CRA.

@ndelangen ndelangen merged commit dfc9ee6 into next Jun 2, 2020
@ndelangen ndelangen deleted the mrmckeb/issue10943 branch June 2, 2020 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants