Take "target" from base tsconfig when present #55
Conversation
|
Hm, yeah, the upgrade story here is a little bit concerning, and it seems like a small thing to do a semver major bump for (though that's not out of the question, of course). Maybe we could have tshy check to see if |
|
@isaacs one way we could also do it not to break people is to have it opt in like other configuration items like |
|
I don't have a strong opinion here, other than that I agree it seems like a small thing to do a breaking change for. Out of the options, I lean towards setting the target only if it's not set in the tsconfig -- I think that's the most intuitive behavior we'd be able to get without breaking. Let me have a go at implementing that. Otherwise if going with an extra configuration option for this I would say let's just have the option be "target", defaulting to es2022, and continue to ignore the tsconfig field entirely as we do today. |
timovv
force-pushed
the
remove-target-from-build-json
branch
from
b852694
to
79ae570
Compare
timovv
changed the title
|
OK, I have changed this so the behavior is:
|
| @@ -116,7 +116,6 @@ Object { | |||
| "module": "nodenext", | |||
| "moduleResolution": "nodenext", | |||
| "rootDir": "../src", | |||
| "target": "es2022", | |||
There was a problem hiding this comment.
Should we have tests for whether we preserve the target and when we don't?
There was a problem hiding this comment.
The existing test case actually covers that case already, since it
- Generates everything from scratch, including the recommended tsconfig.json (which sets target to es2022)
- Then it makes a snapshot -- this is the snapshot you're commenting on, and the target field is no longer present, as expected with the new behavior
- Then the tsconfig.json is overwritten with a custom configuration, which omits the target field
- Finally it takes another snapshot, which is there is no diff for since target is still being set to es2022 as it used to be
timovv
force-pushed
the
remove-target-from-build-json
branch
from
79ae570
to
c887959
Compare
| target: | ||
| readTypescriptConfig().options.target === undefined | ||
| ? 'es2022' | ||
| : undefined, |
There was a problem hiding this comment.
| : undefined, | |
| target: readTypeScriptConfig().options.target ?? 'es2022' |
There was a problem hiding this comment.
What we're doing here is subtly different from that:
- if the target is undefined in the tsconfig.json, then we set es2022
- if the target is defined, then we don't set it at all, so that it inherits from the base tsconfig.
With your suggestion we are instead setting the target in build.json to be the same as in the base tsconfig -- we could do that, but then we'd need to pull in a bit more logic from TS to stringify the value again (the parsed value is actually a ScriptTarget, which TS defines as a numeric enum).
There was a problem hiding this comment.
Oic, that makes sense. Thanks for the clarification.
|
This looks great. Minor style nit, I can get to it if you don't before it lands. Thanks! |
|
@isaacs: just wanted to check if there's anything outstanding we can help with to get this in? We have some customers waiting on the fix, we're happy to vendor it in the interim if need be to unblock them but would be great to see this upstream :) |
isaacs
force-pushed
the
remove-target-from-build-json
branch
from
c887959
to
1b4b73a
Compare
### Packages impacted by this PR - Core packages using ESM migration ### Issues associated with this PR - #28940 - #28918 ### Describe the problem that is addressed by this PR Upgrade to new version of `tshy` which does not force `target` to `ES2022` (see [PR](isaacs/tshy#55)). This should resolve some issues for people whose environments do not support newer syntax (e.g. Node 14 and the Webpack 4 bundler).
Following up on #54 with the suggested fix. It works for us when we try to build in our Azure SDK repo, but unsure if it would cause issues for folks who have their own tsconfig.json with target unset and are relying on tshy to set the target?