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

build: refine project references #699

Closed

Conversation

johnhooks
Copy link
Contributor

@johnhooks johnhooks commented Dec 12, 2022

After reading an article from Shopify and how they implemented ts project references, I learned a few things I wanted to apply. It should specifically speed up VSCode, and possibly any program using the ts lsp. reference

The big suggestion from the article was that the top level tsconfig.json should be able to build the whole project, thats how VSCode will use it. I moved everything shared to a tsconfig.base.json and removed the tsconfig.declaration.json because that didn't have a purpose with a shared base between root level and leaf level tsconfig.json files. It also required a tsconfig.json inside the packages directory, and some modifications to the one in the test directory.

@vercel
Copy link

vercel bot commented Dec 12, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
dinerojs ❌ Failed (Inspect) Dec 31, 2022 at 5:56PM (UTC)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 12, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 23e4e56:

Sandbox Source
@dinero.js/example-cart-react Configuration
@dinero.js/example-cart-vue Configuration
@dinero.js/example-pricing-react Configuration
@dinero.js/example-starter Configuration

.eslintrc.js Outdated
},
},
{
files: ['**/__tests__/**'],
rules: {
'functional/no-expression-statement': ['off'],
'import/no-extraneous-dependencies': ['off'],
'import/no-named-as-default': 'off',
Copy link
Contributor Author

@johnhooks johnhooks Dec 12, 2022

Choose a reason for hiding this comment

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

Fixing the project references and including project: ['./'] in .eslint.js (so it could find the tsconfig paths) must have let the eslint-plugin-import better analyze the import statements, because it started failing this import.

import Big from 'big.js`;

The rule import/no-named-as-default prefers:

import { Big } from 'big.js';

So we can change it or disable the rule.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer fixing the imports and keeping the rule on. If there are imports that can't be fixed, we can ignore the rule locally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will change it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@johnhooks This should be the last change before we can merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know why I periodically had this error happening. But I'll look locally for the solution because it isn't in CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is hard jumping between this and #667, I hadn't removed the rule as you suggested. THAT made the error come back lol.

@johnhooks johnhooks force-pushed the build/refine-project-references branch from e12462e to f3019e2 Compare December 12, 2022 04:50
package.json Show resolved Hide resolved
test/tsconfig.json Outdated Show resolved Hide resolved
website/pages/docs/[[...slug]].tsx Show resolved Hide resolved
Copy link
Collaborator

@sarahdayan sarahdayan left a comment

Choose a reason for hiding this comment

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

Overall it looks great to me. I'll let you handle the final comments @johnhooks and we'll be good to merge.

tsconfig.base.json Outdated Show resolved Hide resolved
test/tsconfig.json Outdated Show resolved Hide resolved
@johnhooks
Copy link
Contributor Author

johnhooks commented Dec 21, 2022

@sarahdayan I made one last edit to packages/dinero.js/src/api/__tests__/config.json. Importing test-utils in the tests was raising an ESLint import/no-unresolved error. I think some of the configOptions in package level config caused the problem. I extended tsconfig.base.json in the root directory and everything works correctly.

Edit: That actually didn't resolve the issue... I'm looking into it. Do you that error in VSCode? What is weird yarn lint doesn't flag it as a problem.

Edit 2: I walked that back, it wasn't necessary, just an issue with my setup.

@sarahdayan
Copy link
Collaborator

sarahdayan commented Dec 21, 2022

@johnhooks Not seeing any issue locally, and the build seems fine in CI.

@johnhooks
Copy link
Contributor Author

johnhooks commented Dec 21, 2022

Thanks I'll start trying to figure out what is wrong on my system and stop looking in the code.

Edit: I was from jumping between this branch and #667 and not running yarn install 🤦. The newer version of eslint-import-resolver-typescript is definitely necessary for this config to work lol.

@johnhooks johnhooks force-pushed the build/refine-project-references branch from 82739f1 to 25e2ab5 Compare December 21, 2022 12:43
@johnhooks
Copy link
Contributor Author

johnhooks commented Dec 21, 2022

There are some changes that need to happen in #667 for this to work. Once this is merged, I'll resolve them.

.eslintrc.js Outdated
},
},
{
files: ['**/__tests__/**'],
rules: {
'functional/no-expression-statement': ['off'],
'import/no-extraneous-dependencies': ['off'],
'import/no-named-as-default': 'off',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sarahdayan with was the last edit from a previous comment. To fix this ESLint issue rather than ignore it.

@johnhooks
Copy link
Contributor Author

johnhooks commented Dec 31, 2022

@sarahdayan I'm having some trouble with getting ESLint to recognize the paths property in tsconfig.base.json. From what I've learned, tsc can compile everything in the current mono repo structure, though it isn't the ideal setup. Ideally the tsconfig.json property rootDir would be the top of the mono repo, but that complicates outputting the files correctly into the packages/*/lib because it will add path of the rootDir to the build output location, like packages/*/lib/packages/*/lib/. There are some hacky solutions I have found, but I don't like any of them.

The simplest solution would be to create a separate packages/test-utils and then import it though the name @dinero.js/test-utils. This would import in ESLint okay because Yarn would have linked the package into node_modules and ESLint can easily find it.

Otherwise we can hold off on this. I am still learning a lot about tsconfig.json setups. I'm currently setting up a few other projects and using a different project structure.

After reading an article from Shopify and how they implemented ts
project references, I learned a few things I wanted to apply. It should
specifically speed up VSCode, and possibly any program using the ts lsp.
[reference](https://shopify.engineering/migrating-large-typescript-codebases-project-references)

build(tsc): update test:types script

To use the project references the command `tsc -b` needs to be used,
not `tsc --noEmit`. It will build the declarations, but will output to
the lib folders in all the relevant packages, which are ignored from git

build: add eslint-import-resolver-typescript
The new tsconfig file in the packages directory was causing an error
because the docs route was trying to load it as a package. This fixes
the issue, but not very elegantly.
@johnhooks
Copy link
Contributor Author

@sarahdayan I'm going to rework this. Do you want me to close it and open it back up when it's ready?

@sarahdayan
Copy link
Collaborator

@johnhooks Yes, feel free to close and reopen when you're ready so we can keep the PR list clear 🙂

@johnhooks johnhooks closed this Feb 18, 2023
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.

None yet

2 participants