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

refactor: only use esm import syntax #1114

Closed
wants to merge 1 commit into from
Closed

Conversation

shuowu
Copy link
Contributor

@shuowu shuowu commented Feb 15, 2022

resolves #1073

  • adds jest tests to validate generated outputs (esm, cjs)
  • uses vite instead of CRA for react-oie test app, now it's faster dev experience and able to test (discover issues) ESM module
  • updates rollup config to better support local development
  • exposes esm node bundle

Downstream tests (green):

@shuowu shuowu mentioned this pull request Feb 15, 2022
@shuowu shuowu marked this pull request as draft February 15, 2022 14:27
@codecov-commenter
Copy link

codecov-commenter commented Feb 15, 2022

Codecov Report

Merging #1114 (afe896e) into master (da969b7) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1114   +/-   ##
=======================================
  Coverage   93.51%   93.51%           
=======================================
  Files         154      154           
  Lines        4147     4149    +2     
  Branches      906      907    +1     
=======================================
+ Hits         3878     3880    +2     
  Misses        253      253           
  Partials       16       16           
Impacted Files Coverage Δ
lib/AuthStateManager.ts 93.87% <100.00%> (ø)
lib/OktaAuth.ts 89.66% <100.00%> (ø)
lib/browser/browserStorage.ts 93.98% <100.00%> (+0.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da969b7...afe896e. Read the comment docs.

@shuowu shuowu marked this pull request as ready for review February 15, 2022 19:46
@@ -247,6 +245,10 @@ var storageUtil: BrowserStorageUtil = {
},

get: function(name: string): string {
// return all cookies when no args is provided
Copy link
Contributor

Choose a reason for hiding this comment

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

If name is optional, should the method signature be:

get: function(name?: string): string


const env = {};
// List of environment variables made available to the app
['ISSUER', 'CLIENT_ID'].forEach((key) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

include 'SPA_CLIENT_ID' for consistency across other sdks

add cjs bundle validation test

use vite for react-oie test app

adjust rollup config for development

generate esm node bundle

add exports field as guiduice of different envs
eng-prod-CI-bot-okta pushed a commit that referenced this pull request Mar 3, 2022
add cjs bundle validation test

use vite for react-oie test app

adjust rollup config for development

generate esm node bundle

add exports field as guiduice of different envs

OKTA-405564
<<<Jenkins Check-In of Tested SHA: 0dcad6d for eng_productivity_ci_bot_okta@okta.com>>>
Artifact: okta-auth-js
Files changed count: 38
PR Link: "#1114"
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.

ESM build contains keyword require crash web app
5 participants