-
Notifications
You must be signed in to change notification settings - Fork 26
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 the examples #205
Fix the examples #205
Conversation
Since the migration to TypeScript, examples had to be adjusted. The SDK now uses named exports. Also the SDK needs to be built before examples can be run. This is now displayed on top of the example instructions. Since the SDK now used package exports, the examples can now import the package using its name instead of relative imports. Some ESLint warnings have been disabled for the examples. Incorrect paths to example fixtures have been fixed. The examples are now type-checked using JSDoc based type annotations. Closes #204
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #205 +/- ##
=======================================
Coverage 68.75% 68.75%
=======================================
Files 6 6
Lines 624 624
Branches 123 123
=======================================
Hits 429 429
Misses 195 195
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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 this quick PR! It looks good to me and I just have a minor question.
authSecret: /** @type {string} */ (process.env.TRANSLOADIT_SECRET), | ||
// authKey: /** @type {string} */ (process.env.API2_SYSTEMTEST_AUTH_KEY), | ||
// authSecret: /** @type {string} */ (process.env.API2_SYSTEMTEST_SECRET_KEY), | ||
// endpoint: /** @type {string} */ ('https://api2-vbox.transloadit.com'), |
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.
The three values that are commented out are the result from internal from other repositories and don't serve a purpose for our users. Should we just remove them here?
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 kept it as-is, but I agree this should probably be removed.
Since the migration to TypeScript, examples had to be adjusted. The SDK now uses named exports. Also the SDK needs to be built before examples can be run. This is now displayed on top of the example instructions.
Since the SDK now used package exports, the examples can now import the package using its name instead of relative imports.
Some ESLint warnings have been disabled for the examples.
Incorrect paths to example fixtures have been fixed.
The examples are now type-checked using JSDoc based type annotations.
Closes #204