-
Notifications
You must be signed in to change notification settings - Fork 257
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
feat: add typescript declarations #413
Conversation
332b833
to
040185d
Compare
91a192b
to
2bf1ec1
Compare
61a9116
to
9ff03dd
Compare
45b2dff
to
1c9ab9e
Compare
9ff03dd
to
e8dbc72
Compare
lib/browser/index.js
Outdated
export { OktaAuthBrowser as OktaAuth }; |
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.
nit: some syntax sugar, export { default as OktaAuth } from './browser';
lib/clock.ts
Outdated
} | ||
|
||
// factory method. Create an instance of a clock from current context. | ||
SdkClock.create = function(/* sdk, options */) { |
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.
keep it static in class?
lib/errors/AuthApiError.js
Outdated
constructor(err, xhr = null) { | ||
const message = err.errorSummary; | ||
super(message); // 'Error' breaks prototype chain here | ||
Object.setPrototypeOf(this, new.target.prototype); // restore prototype chain |
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.
Not sure what's the issue with extending the Error class, but looks like there is a hack needed to make it work. Wondering if we can use any babel plugin to handle it or move the logic to a BaseError
class to make future changes easier?
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.
A search for "typescript extend error prototype" will show more information on the issue, here is an example that also shows this solution: https://stackoverflow.com/questions/41102060/typescript-extending-error-class
A base CustomError
class could possibly make this slightly simpler.
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'll vote for a base error class, also I think it would be better to add the StackOverflow link in code.
lib/server/index.js
Outdated
export { OktaAuthNode as OktaAuth }; |
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.
nit: export { default as OktaAuth } from './server';
import { getUserAgent } from '../builderUtil'; | ||
import serverStorage from './serverStorage'; | ||
import PACKAGE_JSON from '../../package.json'; | ||
const PACKAGE_JSON = require('../../package.json'); |
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.
import * as PACKAGE_JSON from '../../package.json'
may work
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.
In this case, the issue is that importing a JSON file via typescript requires the esModuleInterop
flag to be turned on. I specifically want this flag off because if we have it on the requirement of using this flag is passed along to other users of our ES module, such as okta-angular
.
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.
So the same issue for node-cache
?
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.
Yes, node-cache
is a commonJS
module. It can only be imported by using the allowSyntheticDefaultImports
and esModuleInterop
flags.
@@ -11,26 +11,15 @@ var commonConfig = require('./webpack.common.config'); | |||
var license = fs.readFileSync('lib/license-header.txt', 'utf8'); | |||
|
|||
module.exports = _.extend({}, _.cloneDeep(commonConfig), { | |||
entry: './lib/browser/index.js', | |||
mode: 'production', |
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.
hah, I was meaning this flag in the last review.
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.
Yes webpack was upgraded to version 4 in this PR
Great progress with Typescript! Several comments here:
|
There is a breaking change. We have switched from a default export to named exports. So commonJS code like
Similarly imports of This is necessary to provide types as separate named exports, and to enable tree-shaking.
Files which are used by NodeJS are retaining ES5 compatibility. This is not code in our library, but as part of config, test runners or dev servers, etc. Eventually we will probably convert all library code to .ts. However, the .js files are being processed by Typescript compiler. The rule is we must convert to .ts as soon as we use some Typescript syntax, such as return value or optional params. Since I will be addressing other comments, I may just finish conversion of the remaining files under /lib in this PR. The unit tests, as well, would have some value in being converted to .ts as we would get some extra type checking. However I did not want to change the unit tests in this PR, Since this is a large refactor, I am counting on the unit tests to sanity check that we haven't broken anything. I did convert the E2E test app to Typescript, since this validates that our exported types are consumable from another app. |
@shuowu feedback addressed, also converted remaining files under lib/ to .ts extension |
LGTM! |
@@ -16,6 +19,14 @@ | |||
"setInterval": true, | |||
"clearInterval": true | |||
}, | |||
"settings": { | |||
"jsdoc": { |
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.
Should we have updates to README or CONTRIBUTING regarding the typescript? Is the build/inclusion process any different?
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.
Good catch. There should be some updates to the README. Maybe a "migrating from 3.x" section. I will add 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.
@swiftone Please take a look at changes added to README
README.md
Outdated
@@ -24,10 +25,13 @@ You can learn more on the [Okta + JavaScript][lang-landing] page in our document | |||
|
|||
This library uses semantic versioning and follows Okta's [library version policy](https://developer.okta.com/code/library-versions/). | |||
|
|||
## Release Status | |||
|
|||
:heavy_check_mark: The current stable major version series is: `3.x` |
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.
Missed this line
README.md
Outdated
:heavy_check_mark: The current stable major version series is: `3.x` | ||
|
||
| Version | Status | | ||
| ------- | -------------------------------- | | ||
| `4.x` | :fire: Latest release | | ||
| `3.x` | :heavy_check_mark: Stable | |
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.
shouldn't 3.x get a retirement date in ~9 months?
* Now using named exports. You should change code like | ||
|
||
```javascript | ||
import OktaAuth from '@okta/okta-auth-js' |
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.
Perhaps put a comment in, like: // previous 3.x way
- people will skim for the code blocks and may not notice that this is the "old" block, not the "new" block
README.md
Outdated
const OktaAuth = require('@okta/okta-auth-js').OktaAuth; | ||
``` | ||
|
||
* Typescript definitions are now included. If you were providing your own definitions for `OktaAuth` you should remove these in favor of the types exported by this library. |
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'd put in a mention that these are optional to reassure people who don't want TS
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.
A few nits on the README
* feat: support typescript, use named exports
* feat: support typescript, use named exports
* feat: support typescript, use named exports
* feat: support typescript, use named exports
* feat: support typescript, use named exports
* feat: support typescript, use named exports
* feat: support typescript, use named exports
* bump version to 4.0.0 * ES6 Refactor (#388) * move module from packages to repo root * rename browser/server index * update eslint, use ecmaVersion 2020 * feat: add typescript declarations (#413) * feat: support typescript, use named exports * removes "onSessionExpired" option (#444) implements active autoRenew * Builds to "build" folder, CDN content in "build/dist" * Fixes compatibility with CDN / static app
The main visible change from this refactor is that the module uses named exports instead of a default export. This allows users to import the main
OktaAuth
class but also any exported types.