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

fix(tsconfig): support multiple tsconfigs to be specified #327

Merged
merged 11 commits into from
Aug 7, 2023

Conversation

eamodio
Copy link
Contributor

@eamodio eamodio commented Jul 9, 2023

Avoids reusing a previous cache if the tsconfig prop is different

Refs: #325

@privatenumber
Copy link
Owner

Is this happening because you have multiple tsconfigs?

Can you add a test?

@eamodio
Copy link
Contributor Author

eamodio commented Jul 9, 2023

Yeah, I have 3 parallel builds, and each has its own tsconfig.

As for adding a test -- any guidance there?

@privatenumber
Copy link
Owner

privatenumber commented Jul 10, 2023

Specifically, it sounds like your issue is with multiple tsconfig.json files. The bug sounds like it could happen with one Webpack build, irrelevant of parallel.

I would just follow the existing tests and add multiple tsconfig files.

@eamodio
Copy link
Contributor Author

eamodio commented Jul 10, 2023

I tried getting a test setup, but am not having any luck even getting the other tests to work -- the webpack4/5 ones work, but after that I get failures:

node:internal/crypto/hash:71
  this[kHandle] = new _Hash(algorithm, xofLen);
                  ^

Error: error:0308010C:digital envelope routines::unsupported
    at new Hash (node:internal/crypto/hash:71:19)
    at Object.createHash (node:crypto:133:10)
    at module.exports (c:\Users\Eric\code\eamodio\esbuild-loader\node_modules\.pnpm\[email protected][email protected]\node_modules\webpack\lib\util\createHash.js:135:53)
    at NormalModule._initBuildHash (c:\Users\Eric\code\eamodio\esbuild-loader\node_modules\.pnpm\[email protected][email protected]\node_modules\webpack\lib\NormalModule.js:417:16)
    at handleParseError (c:\Users\Eric\code\eamodio\esbuild-loader\node_modules\.pnpm\[email protected][email protected]\node_modules\webpack\lib\NormalModule.js:471:10)
    at <anonymous> (c:\Users\Eric\code\eamodio\esbuild-loader\node_modules\.pnpm\[email protected][email protected]\node_modules\webpack\lib\NormalModule.js:503:5)
    at <anonymous> (c:\Users\Eric\code\eamodio\esbuild-loader\node_modules\.pnpm\[email protected][email protected]\node_modules\webpack\lib\NormalModule.js:358:12)
    at <anonymous> (c:\Users\Eric\code\eamodio\esbuild-loader\node_modules\.pnpm\[email protected]\node_modules\loader-runner\lib\LoaderRunner.js:373:3)
    at iterateNormalLoaders (c:\Users\Eric\code\eamodio\esbuild-loader\node_modules\.pnpm\[email protected]\node_modules\loader-runner\lib\LoaderRunner.js:214:10)
    at Function.<anonymous> (c:\Users\Eric\code\eamodio\esbuild-loader\node_modules\.pnpm\[email protected]\node_modules\loader-runner\lib\LoaderRunner.js:205:4) {
  opensslErrorStack: [ 'error:03000086:digital envelope routines::initialization error' ],
  library: 'digital envelope routines',
  reason: 'unsupported',
  code: 'ERR_OSSL_EVP_UNSUPPORTED'
}

Node.js v18.16.1
 ELIFECYCLE  Test failed. See above for more details.

There are also no docs for setting things up, running builds or tests, so I'm guessing I've set it up wrong (or its my Node version).

If you could help set this up it would be greatly appreciated. Thanks!

@privatenumber
Copy link
Owner

GitHub Actions runs the tests and they're passing.

Take a look at the setup:
https://github.com/esbuild-kit/esbuild-loader/blob/3ee8b5765aa229cf46c28898a8c22a71d9f5e6e4/.github/workflows/test.yml#L36-L39

@eamodio
Copy link
Contributor Author

eamodio commented Jul 10, 2023

As I mentioned, I've tried and spent a bunch of time on it and haven't been able to get it working. Also, I'm unsure about how best to observe this change in a unit test, as the only way I could see this is to build something (maybe even the same thing) with different tsconfigs which would result in differing outputs.

And to be completely honest, I don't have more time to spend on this issue. Hopefully this can either be merged without a test, or that you or another contributor could create it.

@eamodio
Copy link
Contributor Author

eamodio commented Jul 14, 2023

I was finally able to get a test working with an observable output change. The test fails before my fix and succeeds after it. It also proved that it doesn't have to do with parallel builds just multiple tsconfigs -- which IMO makes this bug even worse.

@eamodio eamodio changed the title Fixes #325 ensures correct caching in parallel webpack builds Fixes #325 ensures correct caching with multiple tsconfig's Jul 14, 2023
@eamodio

This comment was marked as spam.

@eamodio

This comment was marked as spam.

@eamodio

This comment was marked as spam.

@privatenumber
Copy link
Owner

Thanks for the PR.

I removed the caching for a bit. I plan on implementing it in get-tsconfig.

@privatenumber privatenumber changed the title Fixes #325 ensures correct caching with multiple tsconfig's fix(tsconfig): support multiple tsconfigs to be specified Aug 7, 2023
@privatenumber
Copy link
Owner

BREAKING CHANGE: tsconfig passed in by path is now applied regardless of whether it matches

@privatenumber privatenumber changed the base branch from develop to v4 August 7, 2023 19:47
@privatenumber privatenumber merged commit c9eb8df into privatenumber:v4 Aug 7, 2023
1 check passed
@privatenumber
Copy link
Owner

Preparing the v4 release. Would you mind helping test?

npm install -D 'esbuild-kit/esbuild-loader#npm/v4'

@eamodio
Copy link
Contributor Author

eamodio commented Aug 8, 2023

Tried it out and works in my testing! TY

privatenumber pushed a commit that referenced this pull request Aug 9, 2023
BREAKING CHANGE: tsconfig passed in by path is now applied regardless of whether it matches
privatenumber pushed a commit that referenced this pull request Aug 9, 2023
BREAKING CHANGE: tsconfig passed in by path is now applied regardless of whether it matches
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.

2 participants