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

Move to TypeScript #104

Merged
merged 33 commits into from
Jul 3, 2020
Merged

Conversation

sneljo1
Copy link
Contributor

@sneljo1 sneljo1 commented Feb 23, 2020

Got everything sorted out as requested. I did not add dot paths yet because we should get a good base first. That should be a seperate PR.

Everything is nicely typed now. The only thing that break for now, and I want your opinion on this first, is the schema. The schema now requires the enum JSONSchemaType for defining the types. I feel like that might be annoying for users, they will need to fix all their schemas. WDYT?

image

Fixes #86


IssueHunt Summary

Referenced issues

This pull request has been submitted to:


IssueHunt has been backed by the following sponsors. Become a sponsor

@sindresorhus
Copy link
Owner

Make sure you look at and address all the feedback in the original PR. For example: #96 (comment)

source/types.ts Outdated Show resolved Hide resolved
source/conf.ts Outdated Show resolved Hide resolved
source/conf.ts Outdated Show resolved Hide resolved
source/conf.ts Outdated Show resolved Hide resolved
source/types.ts Outdated Show resolved Hide resolved
source/types.ts Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
source/conf.ts Outdated Show resolved Hide resolved
source/conf.ts Outdated Show resolved Hide resolved
source/conf.ts Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

The schema now requires the enum JSONSchemaType for defining the types.

What changed that makes it require this?

@sneljo1
Copy link
Contributor Author

sneljo1 commented Feb 24, 2020

The schema now requires the enum JSONSchemaType for defining the types.

What changed that makes it require this?

Allright, nevermind. This is also a typing issue. Because it does support strings. I will look at your comments later today.

@sneljo1
Copy link
Contributor Author

sneljo1 commented Feb 25, 2020

All comments have been resolved. Tests seem to be fine, except for the Failed to load plugin 'unicorn' declared in 'BaseConfig error on Node 8.

@sindresorhus
Copy link
Owner

I don't see #104 (comment) being handled.

@sindresorhus
Copy link
Owner

This is not done: #104 (comment)

@sindresorhus
Copy link
Owner

#104 (comment) is not resolved.

package.json Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
source/types.ts Outdated Show resolved Hide resolved
source/types.ts Outdated Show resolved Hide resolved
@sneljo1
Copy link
Contributor Author

sneljo1 commented Apr 6, 2020

I have had other priorities, I will pick this up in the week of the 20th of April

@sneljo1
Copy link
Contributor Author

sneljo1 commented May 10, 2020

I've dealt with the fixes. I have also applied all the changes that happend on master to the typescript version. As per 5d25f2f and dd88556

source/types.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
source/index.ts Outdated Show resolved Hide resolved
@sindresorhus sindresorhus changed the title Refactor/typescript Move to TypeScript May 24, 2020
@sneljo1 sneljo1 requested a review from sindresorhus May 24, 2020 10:20
@sindresorhus
Copy link
Owner

Tests are not passing. Try removing xo from https://github.com/sindresorhus/conf/pull/104/files#diff-b9cfc7f2cdf78a7f4b91a753d10865a2R18 and run npm test locally and ensure it passes.

source/index.ts Outdated Show resolved Hide resolved
source/index.ts Outdated Show resolved Hide resolved
@sneljo1
Copy link
Contributor Author

sneljo1 commented Jun 7, 2020

I think it's ok now. The tests weren't working, implemented them like in https://github.com/sindresorhus/got/. The linter issues are fixed and I improved the typing for .get

@sindresorhus
Copy link
Owner

Can you add a type test to confirm #104 (comment) is working?

@sindresorhus
Copy link
Owner

Travis is still failing.

@sneljo1
Copy link
Contributor Author

sneljo1 commented Jul 2, 2020

Allright, looked at it again and fixed the problem. But there are still some issues.

  1. The xo package requires get-stdin which has a await, so either we have to upgrade the node version or downgrade xo
  2. Then, I have this on my mac as well. The 2 watch tests fail. I debugged this to being fs.watch not working properly. This does work when using fs.watchFile. It's really strange. I'm not sure what the difference is since it is just a node module. We now do require node 8.3 instead of 8 for on of the xo rules, but I don't think that should be the problem.

@sindresorhus
Copy link
Owner

  1. I plan to require Node.js 10 after this PR.
  2. This is failing in master too. You don't have to worry about that. Probably related to fs,macos: fs.watch() behavior change in node >= 10.16.0 nodejs/node#29460

@sindresorhus sindresorhus merged commit 41b0c10 into sindresorhus:master Jul 3, 2020
@sindresorhus
Copy link
Owner

Thanks for working on this! Looks great now 🙌🏻

@sneljo1
Copy link
Contributor Author

sneljo1 commented Jul 3, 2020

Ah allright, glad I could help. I will be able to benefit from this as well in my app 🙂

Comment on lines +18 to +20
"prepare": "npm run build",
"build": "del dist && tsc",
"prepublishOnly": "npm run build"
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I know prepare is meant to replace prepublishOnly so you don't need both.

However I've been using the more specific prepack which feels like it's the exact moment when files should be built: right before the tar.gz is created.

Copy link
Owner

Choose a reason for hiding this comment

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

As far as I know prepare is meant to replace prepublishOnly so you don't need both.

Yes, but it's a mess since old npm version use prepare, new ones use prepublishOnly, and future ones will use prepare but with it will work differently. I'm not sure why we use both here. Maybe look at the git commits/blame.

However I've been using the more specific prepack which feels like it's the exact moment when files should be built: right before the tar.gz is created.

Does that one also work when users install a Git commit hash directly? npm i sindresorhus/conf#ad32d2

Copy link
Contributor

Choose a reason for hiding this comment

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

prepack: run BEFORE a tarball is packed (on npm pack, npm publish, and when installing git dependencies)

Copy link
Contributor

Choose a reason for hiding this comment

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

Compared with:

prepare: Run both BEFORE the package is packed and published, on local npm install without any arguments, and when installing git dependencies (See below). This is run AFTER prepublish, but BEFORE prepublishOnly.

Copy link
Owner

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve the TypeScript types
4 participants