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: added ci check for declaartions and added fix for the same #827

Closed

Conversation

saurav0705
Copy link
Contributor

@saurav0705 saurav0705 commented Jul 12, 2023

Issue

In the latest version for code-push web sdk no declarations files are committed due to which ts is failing.

Fix

Added outDir addition option in tsconfig-release.json which fixes the issue
image

Prevention

Added a build check so that this should not happen in future releases

image

@saurav0705 saurav0705 requested a review from a team as a code owner July 12, 2023 14:21
@DmitriyKirakosyan
Copy link
Contributor

@saurav0705 , the check passes for me even without the change (adding outDir) in this PR.

@saurav0705
Copy link
Contributor Author

@saurav0705 , the check passes for me even without the change (adding outDir) in this PR.

Hi @DmitriyKirakosyan, yes I checked this I was able to reproduce this previously and not now so might be a cache issue but can we add this check so this doesn't happen in future

@DmitriyKirakosyan
Copy link
Contributor

@saurav0705 , if the check doesn't test the added changes, it might be more confusing than helpful. It would be great if you could modify the check to ensure it fails when outDir is absent.

@saurav0705
Copy link
Contributor Author

saurav0705 commented Oct 31, 2023

@saurav0705 , if the check doesn't test the added changes, it might be more confusing than helpful. It would be great if you could modify the check to ensure it fails when outDir is absent.

@DmitriyKirakosyan issue is that the package that has been published to npmjs has been generated using command npm run build and not npm run build:release due to which compiler options don't have the declarations : true due to which the .d.ts file is not generated

I think making an action that automatically publishes to npmjs so that this doesn't happen
Also can you look into this #828
as this is the latest PR and also have a command that can be run before publishing to artifactory

@velimir-jankovic
Copy link

@saurav0705 , if the check doesn't test the added changes, it might be more confusing than helpful. It would be great if you could modify the check to ensure it fails when outDir is absent.

@DmitriyKirakosyan issue is that the package that has been published to npmjs has been generated using command npm run build and not npm run build:release due to which compiler options don't have the declarations : true due to which the .d.ts file is not generated

I think making an action that automatically publishes to npmjs so that this doesn't happen Also can you look into this #828 as this is the latest PR and also have a command that can be run before publishing to artifactory

As already discussed, changes in this PR won't ensure release without definition files will be created.
We have internal pipeline for releases, I will make necessary changes there.

Closing.

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

3 participants