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

Batch repo enhancements #225

Closed
7 of 8 tasks
o-alexandrov opened this issue Nov 17, 2020 · 12 comments
Closed
7 of 8 tasks

Batch repo enhancements #225

o-alexandrov opened this issue Nov 17, 2020 · 12 comments
Labels
wontfix This will not be worked on

Comments

@o-alexandrov
Copy link
Contributor

o-alexandrov commented Nov 17, 2020

Let me please propose batch of changes to the DX, so it'd be easier for others to contribute.
Please let me know what you would approve, so I'd know what to help you with :)

Todo (Not started yet)

  • add automated releases generation and make commits messages more strict
    • as a user of this library, it was a bit time consuming to browse commits to realize what changed between beta versions

Completed

  • use typescript-eslint instead of tslint
    • it's been almost 2 years since the standard in TS linting is ESLint, more info
  • add .log files to .gitignore
    • for example, there's already a mistakenly uploaded yarn-error.log
  • bump prettier & use community defaults
    • let's use the defaults, so it's more likely to please higher % of contributors
  • add .npmignore to exclude unrelated files from the deployed package
    • improve the files + exclude instead
  • remove empty dependencies object from package.json
  • add husky to add git hooks
  • add official funding references of your GitHub sponsors link to the package.json
@colinhacks
Copy link
Owner

This all sounds great to me. I'm very open to a PR for these things. I'm also open to giving you commit access to the repo since you've been contributing to Zod for a long time, let me know 👍

@o-alexandrov
Copy link
Contributor Author

Thank you. If you kindly grant commit access, I plan to carefully follow existing patterns in naming of commits, branches, and will always ask before doing any action for the first time.

I'm glad to contribute these changes in the next 48 hours.

@o-alexandrov
Copy link
Contributor Author

Realized there are two other improvements, copied to the list above:

  • add official funding references of your GitHub sponsors link to the package.json
  • add automated releases generation and make commits messages more strict
    • as a user of this library, it was a bit time consuming to browse commits to realize what changed between beta versions

@jameschensmith
Copy link

@o-alexandrov, are you still working on this? I'd like to contribute on this if possible. @colinhacks, I could start getting separate PRs up for each of the points listed in the initial message. Let me know your thoughts when you get a chance, and I'll get the first PR pushed right away 😊

@o-alexandrov
Copy link
Contributor Author

o-alexandrov commented Dec 4, 2020

@jameschensmith please work on it, if you have time. I’m waiting for a reply on circular dependencies, before I work on this.

@jameschensmith
Copy link

Will do! 😁 I'll get started on it tomorrow morning 👍

@jameschensmith
Copy link

@o-alexandrov & @colinhacks,

Regarding the exclusion of unrelated files from the deployed package. There appears to be some funky things relating to yarn/npm & .npmignore. First, I came across this article, which recommends allow-listing (with files in package.json) over deny-listing first and foremost. Seeing as we are doing this this is good to see 🙂

There is a problem, though, and it's quite messy. From the following two yarn issues reported (1; 2), I notice two problems when trying to use .npmignore or .yarnignore locally: (1) lib/ is ignored by .gitignore, which because of its higher precedence is not ignoring any of the files within it; (2) if files is present in package.json, yarn apparently will only take allow-list rules (e.g. !do-not-ignore-me.js).

You can see the route that some have taken in the issues listed, but my recommendation is this: stick with files, and do the excludes from tsconfig.base.json.

Here's an example of what the exclude content could look like:

{
  "exclude": [
    "./src/**/__tests__",
    "./src/playground.ts",
  ]
}

For examples & tests, we shouldn't need to compile them because we use tools such as TS Node (bug: There's a script with TS Node, but it's not in the dependencies) & TS Jest. Looking forward to your thoughts on which direction to take for this 😊 👍

@o-alexandrov
Copy link
Contributor Author

@jameschensmith thank you very much, just to simplify the review for @colinhacks , I went through your PRs and all of what you did are improvements, imho.

Regarding excludes, also in agreement, in your PR, please also mark /lib instead of lib in files of package.json

@colinhacks
Copy link
Owner

colinhacks commented Dec 8, 2020

@o-alexandrov I removed tslint entirely since I never actually used yarn lint and it wasn't integrated into the build process in any way. So perhaps the first item should be "Add eslint-typescript".

I checked off "funding".

I also checked off "prettier" having just merged @jameschensmith's #260

I also checked off "exclude". It's now using includes (not files) and excludes to prevent unused files from getting deployed. I just merged that change last night.

@o-alexandrov
Copy link
Contributor Author

o-alexandrov commented Dec 8, 2020

@colinhacks Thank you, I also added a couple of other improvements in the 2 PRs above.
Please let me know, if I should have merged the PR with backing improvements myself to not waste your time with it

@colinhacks
Copy link
Owner

That would have been fine! Same for the rest of James's changes.

@stale
Copy link

stale bot commented Mar 2, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Mar 2, 2022
@stale stale bot closed this as completed Mar 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

3 participants