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

Incorrect instance hash for different compiler options #1316

Closed
timocov opened this issue May 12, 2021 · 7 comments · Fixed by #1317
Closed

Incorrect instance hash for different compiler options #1316

timocov opened this issue May 12, 2021 · 7 comments · Fixed by #1317

Comments

@timocov
Copy link
Contributor

timocov commented May 12, 2021

Let's say you have 2 packages, and one of them you want to build with target=es5, another one with target=es6. If you set up ts-loader options to override compiler options (in loader options, not in tsconfig) it wont work properly.

There is a test https://github.com/TypeStrong/ts-loader/tree/main/test/execution-tests/loaderOptions for similar situation, but it doesn't cover this case. Let's say instead of overriding the only getCustomTransformers option you want to override something in compilerOptions. In this case this code will return [object Object] for both different objects (different ref and a value itself) which causes the issue (also this code doesn't cover a case when a value is false, thus if a value is falsy but enabled by default it will also failed).

Not too sure about the fix here. I see different approaches to solve it:

  1. Use JSON.stringify for non-functions and value.toString() otherwise
  2. Allow to provide a instanceKey or something like that so people can declare their own cache key and refer to them in different places

Maybe you have thoughts how to solve it in better way. Let me know you what do you think. I'm ready to make a fix for this.

@timocov
Copy link
Contributor Author

timocov commented May 12, 2021

I just created a PR (draft at the moment) #1317 with potential fix of the issue including tests. I'm not sure about the fix itself, probably we need to traverse the tree if there is an object in options. What do you guys think?

@johnnyreilly
Copy link
Member

probably we need to traverse the tree if there is an object in options.

Perhaps - what would be the impact if you did not?

@timocov
Copy link
Contributor Author

timocov commented May 13, 2021

what would be the impact if you did not?

I guess that it might be right now as well - the hash might be different for 2 the same objects (by value) but with different order of keys:

{ transpileOnly: true, compilerOptions: {} }
// vs
{ compilerOptions: {}, transpileOnly: true }

@johnnyreilly
Copy link
Member

Given this is a niche use case, a simple solution is probably preferable.

johnnyreilly pushed a commit that referenced this issue May 18, 2021
* fix: Fixed generating cache for loading options to handle objects

Fixes #1316

* fix: Updated ts-loader instance hash in tests

* Added changelog and changed version in package.json
@johnnyreilly
Copy link
Member

@timocov
Copy link
Contributor Author

timocov commented May 18, 2021

@johnnyreilly thanks for merging it! Is it possible to provide a backport for v8? We're still on webpack@4 (we're working on migrating to v5, but it's hard).

@timocov
Copy link
Contributor Author

timocov commented May 18, 2021

#1319

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 a pull request may close this issue.

2 participants