-
Notifications
You must be signed in to change notification settings - Fork 97
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
Clean up document #218
Clean up document #218
Conversation
Codecov Report
@@ Coverage Diff @@
## main #218 +/- ##
=======================================
Coverage 79.68% 79.68%
=======================================
Files 49 49
Lines 2820 2820
Branches 469 469
=======================================
Hits 2247 2247
Misses 404 404
Partials 169 169
Continue to review full report at Codecov.
|
7c1cb23
to
ad7eea4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution. We can now manipulate documents in more detail.
I left a minor request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the detailed explanation, it was easier to review. 😄
I was worried about using the firebase-js-sdk
code, so I left a comment, so please check it.
"start": "webpack-dev-server --watch --config webpack.dev.config.js", | ||
"test": "karma start karma.conf.js --single-run", | ||
"test:yorkie.dev": "karma start karma.conf.js --single-run --testRPCAddr=https://yorkie.dev/api", | ||
"test:watch": "karma start karma.conf.js", | ||
"lint": "eslint . --fix --max-warnings=0 --ext .ts", | ||
"prepare": "npm run build", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we removed prepare
? Previously, if the user installs yorkie-js-sdk with npm install
, prepare
is preparing bundles(yorkie-js-sdk.js
, yorkie-js-sdk.d.ts
).
https://docs.npmjs.com/cli/v7/using-npm/scripts#life-cycle-scripts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a look this script https://github.com/yorkie-team/yorkie-js-sdk/blob/main/.github/workflows/npm-publish.yml
I think after we run npm run build
, I think yorkie-js-sdk.js
, yorkie-js-sdk.d.ts
are generated. isn't it? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This script was previously added when removing the /dist folder from the source code. I remember that without the script there was no dist file after npm install from the specific branch directly(e.g. npm install https://github.com/yorkie-team/yorkie-js-sdk#main
). 🤔
https://stackoverflow.com/questions/17509669/how-to-install-an-npm-package-from-github-directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see thank you for letting me know!
…rkie-team/yorkie-js-sdk into soeunlee/181/cleanup-document
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
@hackerwins In the generated docs, could you let me know the part which we should remove? For example, |
Co-authored-by: Youngteac Hong <[email protected]>
What this PR does / why we need it?
Clean up document
Any background context you want to provide?
This is a process for pruning d.ts file and then generating cleaned up document files
I referenced firebase-js-sdk https://github.com/firebase/firebase-js-sdk/blob/master/repo-scripts/prune-dts/prune-dts.ts
and https://github.com/microsoft/rushstack/blob/master/build-tests/api-extractor-scenarios/src/runScenarios.ts
What are the relevant tickets?
Fixes #181
Checklist