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

chore(deps): update deps to latest, drop old beachball and eslint deps, add new eslint config file #73

Merged
merged 12 commits into from
Dec 18, 2024

Conversation

ctran88
Copy link
Contributor

@ctran88 ctran88 commented Dec 17, 2024

What's New?

  • update deps to latest
    • except node-fetch, which causes some import issues in v3
  • since eslint was updated from v8 to v9, had to update config file and rules
  • moved the old e2e tests to be co-located next to their source files since eslint was complaining that it wasn't a part of the project service discovery
  • dropped beachball

Screenshots (if appropriate):

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have manually tested my code thoroughly
  • I have added/updated inline documentation for public facing interfaces if relevant
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing integration and unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Additional context

@ctran88 ctran88 marked this pull request as ready for review December 17, 2024 03:41
CHANGELOG.json Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the data in this file need to be persisted in a different format?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no the last published version already used the contents of this file, which is put into CHANGELOG.md. this is only a cache for beachball

Comment on lines +32 to +33
webauthnDevices: expect.any(Array<WebAuthnDevices>),
webauthnTypes: expect.any(Array<WebAuthnType>),
Copy link
Contributor

Choose a reason for hiding this comment

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

The other matchers here seem to be using constructors of primitive JS types.
What's the expected behavior of an expect.any(Array<FooType>) match?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it matches against an array of objects whose shape (keys and key value types) conform to FooType

@@ -13,7 +13,7 @@ export function apiConfiguration(config?: ConfigurationParameters): Configuratio
fetchApi: fetch as unknown as ConfigurationParameters['fetchApi'],
headers: {
...config?.headers,
'Authorization': `Bearer ${config?.accessToken}`,
'Authorization': `Bearer ${config?.accessToken as string}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the case of an undefined config need to be handled?

& a separate question, but why are accessToken and headers.Authorization both needed in this object?

Copy link
Contributor Author

@ctran88 ctran88 Dec 17, 2024

Choose a reason for hiding this comment

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

apiConfiguration is only ever called internally when constructing codegen options

apiConfiguration: apiConfiguration({

and we take in our own values in PassageFlexConfig to populate this function's parameters

export type PassageFlexConfig = {

where we always throw on undefined for the api key before this function is called

if (!config.apiKey) {
throw Error('A Passage API key is required. Please include {appId: YOUR_APP_ID, apiKey: YOUR_APP_ID}.');

so the value should never be undefined at this point. i changed the parameter to be required in 9b6211c

Copy link
Contributor Author

@ctran88 ctran88 Dec 17, 2024

Choose a reason for hiding this comment

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

why are accessToken and headers.Authorization both needed in this object?

good point, i've removed the header in de63778

@ctran88 ctran88 force-pushed the PSG-5204-update-deps-breaking branch from 6c99916 to 9b6211c Compare December 17, 2024 23:49
Base automatically changed from PSG-5580-drop-commonjs-support to main December 18, 2024 01:25
@ctran88 ctran88 force-pushed the PSG-5204-update-deps-breaking branch from 9b6211c to d64fa3c Compare December 18, 2024 01:28
@ctran88 ctran88 force-pushed the PSG-5204-update-deps-breaking branch from d64fa3c to 82e1b55 Compare December 18, 2024 01:33
@ctran88 ctran88 merged commit 95d3842 into main Dec 18, 2024
4 checks passed
@ctran88 ctran88 deleted the PSG-5204-update-deps-breaking branch December 18, 2024 01:34
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.

2 participants