-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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: deprecate @aws-cdk/yaml-cfn
#14001
Conversation
`aws-cdk-lib` needs to have no dependencies (except for `constructs`), and we don't want to support this package as part of its public API. Therefore, it needs to be bundled into all packages that consume it. NPM has a bug that makes it impossible to `bundledDependencies` a package that is linked from the monorepo. We tried to work around that bug but it blocked our build because the `.tgz` is sometimes not available during the `postpack` phase (#13933). Simplest way around this now is to just copy/paste the implementation of `yaml-cfn` 3 times. Should we ever need to bundle monorepo dependencies again in the future, we can try to revive the `postpack` solution of #13933.
* @param obj the data structure to serialize | ||
* @returns a string containing the YAML representation of {@param obj} | ||
*/ | ||
export function serialize(obj: any): string { |
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.
How about moving this to 'core' and reduce the dupes?
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.
CLI still cannot depend on core
so doesn't help that much.
Also it should not be part of the public API, this package is/was an implementation detail.
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 a strong opinion but) put it in cx-api
but not export it? Usage would be -
import * as yaml_cfn from '@aws-cdk/cx-api/yaml-cfn'
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.
We have tricks/hacks for making things in core
not public. See cfn-parse.ts
, for example.
* @param obj the data structure to serialize | ||
* @returns a string containing the YAML representation of {@param obj} | ||
*/ | ||
export function serialize(obj: any): string { |
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 a strong opinion but) put it in cx-api
but not export it? Usage would be -
import * as yaml_cfn from '@aws-cdk/cx-api/yaml-cfn'
/** | ||
* Deserialize the YAML into the appropriate data structure. | ||
* | ||
* @param str the string containing YAML | ||
* @returns the data structure the YAML represents | ||
* (most often in case of CloudFormation, an object) | ||
*/ | ||
export function deserialize(str: string): any { | ||
return parseYamlStrWithCfnTags(str); | ||
} | ||
|
||
function makeTagForCfnIntrinsic(intrinsicName: string, addFnPrefix: boolean): yaml_types.Schema.CustomTag { | ||
return { | ||
identify(value: any) { return typeof value === 'string'; }, | ||
tag: `!${intrinsicName}`, | ||
resolve: (_doc: yaml.Document, cstNode: yaml_cst.CST.Node) => { | ||
const ret: any = {}; | ||
ret[addFnPrefix ? `Fn::${intrinsicName}` : intrinsicName] = | ||
// the +1 is to account for the ! the short form begins with | ||
parseYamlStrWithCfnTags(cstNode.toString().substring(intrinsicName.length + 1)); | ||
return ret; | ||
}, | ||
}; | ||
} | ||
|
||
const shortForms: yaml_types.Schema.CustomTag[] = [ | ||
'Base64', 'Cidr', 'FindInMap', 'GetAZs', 'ImportValue', 'Join', 'Sub', | ||
'Select', 'Split', 'Transform', 'And', 'Equals', 'If', 'Not', 'Or', 'GetAtt', | ||
].map(name => makeTagForCfnIntrinsic(name, true)).concat( | ||
makeTagForCfnIntrinsic('Ref', false), | ||
makeTagForCfnIntrinsic('Condition', false), | ||
); | ||
|
||
function parseYamlStrWithCfnTags(text: string): any { | ||
return yaml.parse(text, { | ||
customTags: shortForms, | ||
schema: 'core', | ||
}); | ||
} |
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.
Since we only use serialize
in CodeBuild, let's maybe remove all this?
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
`aws-cdk-lib` needs to have no dependencies (except for `constructs`), and we don't want to support this package as part of its public API. Therefore, it needs to be bundled into all packages that consume it. NPM has a bug that makes it impossible to `bundledDependencies` a package that is linked from the monorepo. We tried to work around that bug but it blocked our build because the `.tgz` is sometimes not available during the `postpack` phase (aws#13933). Simplest way around this now is to just copy/paste the implementation of `yaml-cfn` 3 times. Should we ever need to bundle monorepo dependencies again in the future, we can try to revive the `postpack` solution of aws#13933. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
aws-cdk-lib
needs to have no dependencies (except forconstructs
),and we don't want to support this package as part of its public API.
Therefore, it needs to be bundled into all packages that consume it.
NPM has a bug that makes it impossible to
bundledDependencies
apackage that is linked from the monorepo.
We tried to work around that bug but it blocked our build because the
.tgz
is sometimes not available during thepostpack
phase (#13933).Simplest way around this now is to just copy/paste the implementation
of
yaml-cfn
3 times. Should we ever need to bundle monorepodependencies again in the future, we can try to revive the
postpack
solution of #13933.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license