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

TS Type Breaking Changes #3453

Closed
3 tasks done
MrArnoldPalmer opened this issue Sep 17, 2020 · 4 comments · Fixed by #3485
Closed
3 tasks done

TS Type Breaking Changes #3453

MrArnoldPalmer opened this issue Sep 17, 2020 · 4 comments · Fixed by #3485
Assignees
Labels
bug This issue is a bug. needs-triage This issue or PR still needs to be triaged.

Comments

@MrArnoldPalmer
Copy link

Confirm by changing [ ] to [x] below to ensure that it's a bug:

Describe the bug
#3398 resulted in TS libraries consuming the aws-sdk to break if referencing the ConfigurationOptions type.

Is the issue in the browser/Node.js?
Node.js

If on Node.js, are you running this on AWS Lambda?
Yes (and also on users local machines)
Details of the browser/Node.js version
v12.18.2

SDK version number
v2.743.0

To Reproduce (observed behavior)
Create a typescript file w/ sdk dependency v1.739.0
Import ConfigurationOptions like so

import { ConfigurationOptions } from 'aws-sdk/lib/config'

Upgrade to sdk >v1.740.0
Import statement can no longer resolve ConfigurationOptions

Expected behavior
ConfigurationOptions is still available at lib/config entrypoint

Additional context
This is an issue within the aws-cdk. We use ConfigurationOptions in multiple places to expose sdk configuration options to the user in order for them to customize various behaviors.

It is also used within our custom resource provider framework runtime which runs inside the aws lambda runtime.

This poses a couple of issues for us. We can upgrade our code to point to aws-sdk/lib/config-base to reference this object, though if a user is running their code inside lambda we don't control the version of the sdk that is present. We have also considered not importing this type and instead writing our own interface that duplicates it. This obviously isn't ideal as if non-breaking changes are made to the interface in the future within aws-sdk-js, we would have to copy those changes in our code to make the feature available.

Since ConfigurationOptions is exposed on the constructor of the Config class, our assumption has been its okay to expose this type on our public API for use. Nested import paths can obviously be troublesome for refactoring, can ConfigurationOptions be exported from the top level along with other TS types exposed by the public API for library authors to reference? This still will break some of our users if they are using an old version of the sdk with a newer version of the cdk and vice-versa, but at least we could provide a fix path to upgrade one or the other.

@MrArnoldPalmer MrArnoldPalmer added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 17, 2020
MrArnoldPalmer added a commit to aws/aws-cdk that referenced this issue Sep 17, 2020
Upgrade dependency versions when available. Change the yarn-upgrade
workflow to only upgrade minor versions automatically. This will prevent
major version dependency upgrades from happening automatically in the
future.

Exclude `aws-sdk` from automatic due to changes in TS type definitions
that removed `ConfigurationOptions` from the `aws-sdk/lib/config`
entrypoint. See aws/aws-sdk-js#3453 for
details.
MrArnoldPalmer added a commit to aws/aws-cdk that referenced this issue Sep 17, 2020
Upgrade dependency versions when available. Change the yarn-upgrade
workflow to only upgrade minor versions automatically. This will prevent
major version dependency upgrades from happening automatically in the
future.

Exclude `aws-sdk` from automatic upgradesdue to changes in TS type
definitions that removed `ConfigurationOptions` from the
`aws-sdk/lib/config` entrypoint. See
aws/aws-sdk-js#3453 for details.
MrArnoldPalmer added a commit to aws/aws-cdk that referenced this issue Sep 17, 2020
Upgrade dependency versions when available. Change the yarn-upgrade
workflow to only upgrade minor versions automatically. This will prevent
major version dependency upgrades from happening automatically in the
future.

Exclude `aws-sdk` from automatic upgrades temporarily due to changes in
TS type definitions that removed `ConfigurationOptions` from the
`aws-sdk/lib/config` entrypoint. See
aws/aws-sdk-js#3453 for details.
MrArnoldPalmer added a commit to aws/aws-cdk that referenced this issue Sep 17, 2020
Upgrade dependency versions when available. Change the yarn-upgrade
workflow to only upgrade minor versions automatically. This will prevent
major version dependency upgrades from happening automatically in the
future.

Exclude `aws-sdk` from automatic upgrades temporarily due to changes in
TS type definitions that removed `ConfigurationOptions` from the
`aws-sdk/lib/config` entrypoint. See
aws/aws-sdk-js#3453 for details.
MrArnoldPalmer added a commit to aws/aws-cdk that referenced this issue Sep 18, 2020
Upgrade dependency versions when available. Change the yarn-upgrade
workflow to only upgrade minor versions automatically. This will prevent
major version dependency upgrades from happening automatically in the
future.

Exclude `aws-sdk` from automatic upgrades temporarily due to changes in
TS type definitions that removed `ConfigurationOptions` from the
`aws-sdk/lib/config` entrypoint. See
aws/aws-sdk-js#3453 for details.
@AllanZhengYP AllanZhengYP self-assigned this Sep 22, 2020
@AllanZhengYP
Copy link
Contributor

Hi @MrArnoldPalmer

Thank you for bringing this up.

can ConfigurationOptions be exported from the top level along with other TS types exposed by the public API for library authors to reference?

We can expose ConfigurationOptions in root level. I don't expect any breaking change will be introduced to the type. Any required configuration entry will definitely break users. In fact we should have done that before CDK importing the ConfigurationOptions from the path.

@MrArnoldPalmer
Copy link
Author

Thanks @AllanFly120 ! I'm curious if just all interfaces/types that are exposed by exported APIs (whether as arguments, return values, properties, etc) should just be exported from the root to allow library authors the ability to rely on them going forward.

This obviously isn't needed for this specific problem, but it may make for a better experience for TS library authors building on top of the SDK.

@MrArnoldPalmer
Copy link
Author

On that note, RetryDelayOptions is also used in the CDK https://github.com/aws/aws-cdk/blob/master/packages/@aws-cdk/aws-logs/lib/log-retention-provider/index.ts#L6 .

Can that be exported as well?

As far as I can see those are the only ones CDK is using, but I would imagine additional libraries are referencing others.

@AllanZhengYP
Copy link
Contributor

We will re-evaluate all other interfaces later. The idea is still to minimize the surface area.

eladb pushed a commit to cdklabs/decdk that referenced this issue Jan 18, 2022
Upgrade dependency versions when available. Change the yarn-upgrade
workflow to only upgrade minor versions automatically. This will prevent
major version dependency upgrades from happening automatically in the
future.

Exclude `aws-sdk` from automatic upgrades temporarily due to changes in
TS type definitions that removed `ConfigurationOptions` from the
`aws-sdk/lib/config` entrypoint. See
aws/aws-sdk-js#3453 for details.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. needs-triage This issue or PR still needs to be triaged.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants