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

internal(refactor): improving maintainability for V2 release #1400

Merged
merged 255 commits into from Aug 9, 2023

Conversation

carlocorradini
Copy link
Contributor

@carlocorradini carlocorradini commented Dec 19, 2022

This PR starts a series of iterations for improving the maintainability of the project and trying not to create too many conflicts with new-major-release branch.
Note that this PR is not ready to be merged (as written before) because the code style must be applied to the new code present in new-major-release branch and not the master since it can create too many conflicts.
Should fix #1106 and create a base for a better coding experience!
What has been changed:

  1. Updated husky configuration to reflect new version.
  2. Updated lint-staged configuration (replacing TSLint with ESLint) and moved to a proper configuration file.
  3. Updated VS Code settings settings.json to include formatOnSave and defaultFormatter (prettier).
  4. Added VS Code extensions.json which includes a list of recommended extensions to install for improving experience while coding in VS Code (help new contributors with their first contribution).
  5. Updated benchmarks to match ESLint code style.
  6. Moved publish-website.sh to scripts directory. publish-website.sh is POSIX-compliant. scripts include a __commons.sh file (sourced by publish-website.sh) with utility functions (e.g., logger) and more.
  7. Replaced TSLint with ESLint. This has been long awaited! The configuration file must be changed to reflect what @MichalLytek desiders (currently is based on Airbnb code style).
  8. Added markdownlint: A Node.js style checker and lint tool for Markdown/CommonMark files.
  9. Added ShellCheck: A shell script static analysis tool. FUTURE: Add action-shellcheck in CI.
  10. Added CSpell: A Spell Checker for Code! to check for typo(s) and spell checking errors.
  11. Updated Jest configuration file to match ts-jest. Moreover, type checking and hints in the configuration file
  12. Added more scripts in package.json for checking and fixing errors.

@MichalLytek Let me know what you think and what you want to change/update/add/delete.


PS: Do we really need Gulp?

@MichalLytek MichalLytek self-requested a review December 19, 2022 17:05
@MichalLytek MichalLytek added Community 👨‍👧 Something initiated by a community Internal 🏠 All the things related to internal parts labels Dec 19, 2022
@MichalLytek
Copy link
Owner

PS: Do we really need Grunt?

Would be great to prepare a simple POSIX script for building and publishing the package.
TBH the grunt/gulp and other stuff was just inherited from the old routing-controllers project. I had 0 experience with npm then 😅

@carlocorradini
Copy link
Contributor Author

carlocorradini commented Dec 19, 2022

Bye bye Gulp 🥳😅

@carlocorradini
Copy link
Contributor Author

carlocorradini commented Dec 19, 2022

Some things:

  1. Do you have any preference for ESLint and/or code style?
  2. Can you describe, in short, what you want for building and publishing the package? build > package > dist is a little bit confusing ahahah
  3. browser-shim.ts must be copied directly to build directory and browser-shim.d.ts must be removed?

@carlocorradini
Copy link
Contributor Author

@MichalLytek
Gulp has been removed!
Available npm scripts:

  1. build
       Build project. It removes build directory, compiles the project, deletes browser-shim.d.ts(frombuilddirectory), and copiesbrowser-shim.ts(tobuild` directory) when executed.
  2. build:watch
       Build project in watch mode.
  3. check
       Check for errors, etc... (call check:* under the hood).
  4. clean
       Clean generated directories and/or files (e.g. build directory).
  5. docs (UNCHANGED).
  6. fix
       Try to fix errors, etc... (call fix:* under the hood).
  7. test (UNCHANGED).

Note that now, before executing npm publish it automatically runs npm run build. Therefore, to publish a newer version:

  1. Change version in package.json (This can be automated by a POSIX script or a npm script)
  2. Execute npm publish

@carlocorradini carlocorradini marked this pull request as draft December 19, 2022 21:41
…t has been updated to the official index.d.ts types
@carlocorradini
Copy link
Contributor Author

carlocorradini commented Dec 19, 2022

You can now use typescript paths in import statements:
BEFORE:

import { Hello } from '../../src/World';

NOW:

import { Hello } from '~/World';

~/ points to src/
Very useful, but most importantly, you can use it in tests
Note that during compilation, all ~/ imports are translated to relative imports.
Very useful use-case when importing MetadataStorage in global TypeGraphQLMetadataStorage:

declare namespace NodeJS {
  interface Global {
    TypeGraphQLMetadataStorage: import("~/metadata/metadata-storage").MetadataStorage;
    //                                  ^
  }
}

Note that declarations.d.ts has been moved under @types directory and divided into Reflect.d.ts and TypeGraphQL.d.ts. Moreover. Reflect.d.ts has been updated to the latest official types

@carlocorradini
Copy link
Contributor Author

@MichalLytek I can't go any further or I'll create conflict with next-major-release branch.
What do you think of the changes?
Can you try them?

@carlocorradini carlocorradini marked this pull request as ready for review January 8, 2023 14:36
@mmmeff
Copy link

mmmeff commented Jan 12, 2023

@carlocorradini Thank you for doing this work! As someone looking to start contributing to Type-GraphQL, this will go a long way towards my QoL as a contributor. Cheers!

@carlocorradini
Copy link
Contributor Author

@mmmeff Thanks 🥳

@MichalLytek
Copy link
Owner

@carlocorradini

Can you describe, in short, what you want for building and publishing the package? build > package > dist is a little bit confusing ahahah

Basically, my publishing workflow is designed in a way that prevents accidental publishing. That's why I had private: true in the root package.json file.
Secondly, I don't like using root package.json for publishing. I prefer the "0.0.0-unreleased" because when someone opens master branch, there's no confusion that he's browsing v1.2.3 when the feature exists but in his project it throws import error or something.

That's why I wanted to have a simple script, that will clear the output folder, build the code, copy it to package folder and copy other files to publish. That's what the gulp script was doing basically.
Here is an example from typegraphql-prisma repo:
https://github.com/MichalLytek/typegraphql-prisma/blob/main/package.sh

@davingee
Copy link

Is there any way I can contribute? I have checked this branch out and am unable to run the federated v2 example.

npm i
cd into federated example 
npx ts-node ./index.ts
image

I also have some questions about how I would add this

extend schema
  @link(url: "https://specs.apollo.dev/federation/v2.0",
        import: ["@key", "@shareable"])

@MichalLytek
Copy link
Owner

@carlocorradini We should configure typeRoots in tsconfig:
https://typestrong.org/ts-node/docs/troubleshooting#missing-types

But I couldn't make it work 😕

@MichalLytek
Copy link
Owner

@davingee It's added automatically by buildSubgraphSchema - see helpers/buildFederatedSchema

@carlocorradini
Copy link
Contributor Author

@carlocorradini We should configure typeRoots in tsconfig:
https://typestrong.org/ts-node/docs/troubleshooting#missing-types

But I couldn't make it work 😕

I'll check it out

@MichalLytek
Copy link
Owner

Let's merge it and fix all the issues on the fly! 🎉 🥳 🍾

@MichalLytek MichalLytek merged commit 3905582 into MichalLytek:master Aug 9, 2023
9 of 10 checks passed
@MichalLytek MichalLytek added this to the 2.0 release milestone Aug 9, 2023
@MichalLytek MichalLytek added this to In review in Board via automation Aug 9, 2023
@MichalLytek MichalLytek moved this from In review to Done in Board Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community 👨‍👧 Something initiated by a community Internal 🏠 All the things related to internal parts
Projects
Board
  
Done
Development

Successfully merging this pull request may close these issues.

Maintainability improvements.