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: remove JSON syntax error and regenerate tsconfig files #3566

Merged
merged 8 commits into from
Jan 27, 2023

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Jan 25, 2023

Short description of the changes

Regenerated some tsconfig.json file because they are shown as modified by git after npm install.

Refs: #3208

@Flarna Flarna requested a review from a team January 25, 2023 08:55
@codecov
Copy link

codecov bot commented Jan 25, 2023

Codecov Report

Merging #3566 (581b372) into main (5b070b8) will decrease coverage by 3.06%.
The diff coverage is n/a.

❗ Current head 581b372 differs from pull request most recent head f324f22. Consider uploading reports for the commit f324f22 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3566      +/-   ##
==========================================
- Coverage   93.88%   90.83%   -3.06%     
==========================================
  Files         255       77     -178     
  Lines        7757     1670    -6087     
  Branches     1612      338    -1274     
==========================================
- Hits         7283     1517    -5766     
+ Misses        474      153     -321     
Impacted Files Coverage Δ
api/src/platform/node/globalThis.ts 0.00% <0.00%> (-100.00%) ⬇️
...lemetry-resources/src/detectors/ProcessDetector.ts 31.81% <0.00%> (-68.19%) ⬇️
packages/opentelemetry-sdk-trace-web/src/utils.ts 65.83% <0.00%> (-29.20%) ⬇️
...lemetry-resources/src/detectors/BrowserDetector.ts 93.33% <0.00%> (-6.67%) ⬇️
...y-sdk-trace-base/src/export/ConsoleSpanExporter.ts 78.57% <0.00%> (-1.43%) ⬇️
api/src/baggage/utils.ts 100.00% <0.00%> (ø)
...ckages/opentelemetry-core/src/baggage/constants.ts
.../sdk-metrics/src/state/MeterProviderSharedState.ts
...opentelemetry-core/src/platform/node/globalThis.ts
packages/sdk-metrics/src/view/Aggregation.ts
... and 175 more

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Allow JS and include generated are definitely needed for the proto exporter base. it looks like the new autogeneration script overwrites these options assuming they are the same everywhere.

@Flarna
Copy link
Member Author

Flarna commented Jan 27, 2023

Allow JS and include generated are definitely needed for the proto exporter base.

Seems there is a significant gap in tests because CI is green...

@dyladan
Copy link
Member

dyladan commented Jan 27, 2023

Seems there is a significant gap in tests because CI is green...

Yeah I'm looking into it now.

I found the problem which causes the key to be incorrectly overwritten. In https://github.com/open-telemetry/opentelemetry-js/blob/main/scripts/update-ts-configs.js#L249-L258 the JSON is failed to read because there is a trailing , in the tsconfig.esnext.json#include array which is not allowed in JSON. This causes it not to be properly merged. Removing the trailing , results in the expected behavior.

@dyladan
Copy link
Member

dyladan commented Jan 27, 2023

Allow JS and include generated are definitely needed for the proto exporter base.

Seems there is a significant gap in tests because CI is green...

I think the tests are green because this is only happening in the esnext target

@dyladan
Copy link
Member

dyladan commented Jan 27, 2023

This is no longer a chore but a fix because the esnext build is broken until the config generation is fixed. I verified this on my local machine. The esnext build does not contain the generated js files for the protobuf.

@dyladan dyladan self-requested a review January 27, 2023 14:17
@dyladan dyladan changed the title chore: regenerate tsconfig files fix: remove JSON syntax error and regenerate tsconfig files Jan 27, 2023
@dyladan
Copy link
Member

dyladan commented Jan 27, 2023

I went ahead and pushed the fixes so that we can merge and release today

@Flarna
Copy link
Member Author

Flarna commented Jan 27, 2023

I think the tests are green because this is only happening in the esnext target

not related to this pr but I think we should test esnext. Releasing something untested sounds not promising.

@dyladan dyladan merged commit e0abcc0 into open-telemetry:main Jan 27, 2023
@legendecas
Copy link
Member

We should be able to support merging the generated tsconfig with the manually written one: https://github.com/open-telemetry/opentelemetry-js/blob/main/scripts/update-ts-configs.js#L39. The problem here is caused by the silent suppression of parsing error: https://github.com/open-telemetry/opentelemetry-js/blob/main/scripts/update-ts-configs.js#L254.

Maybe we should abort when failed to parse a tsconfig.json.

@Flarna Flarna deleted the update-tsconfigs branch January 27, 2023 19:02
@dyladan dyladan mentioned this pull request Jan 27, 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.

3 participants