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

[WIP] feat!: migrate to constructs #1671

Closed
wants to merge 3 commits into from
Closed

Conversation

Chriscbr
Copy link
Contributor

@Chriscbr Chriscbr commented Mar 8, 2022

Just an experiment.

Known issues:

  • Adding a static "of" method to access a project given a scope seems to be important for components to access common things like dependencies, gitignore, and other project-specific information. I wanted these to all just be named "of" but this causes a compilation error from JSII because the same static method name is used in subclasses but with different return types. See Identical static methods in parent/child classes are mistakenly recognized as invalid in JSII aws/jsii#3410. So for now the names look like "ofTypeScriptProject" etc. which is a bit awkward but I'm not sure if I see a more natural solution.

Open questions:

  • In the future (or even within this PR) we should add a linter that enforces a common constructor structure on components. Right now the component signatures look like constructor(scope: Construct, props: Props) in v1 of this PR.
    • Should the ID be required? as in constructor(scope: Construct, id: string, props: Props) like most other CDK libraries? There is a constraint in constructs that sibling nodes must have different names, and this could get awkward if the IDs are not customizable.
    • Should we allow other kinds of scopes (i.e. project-specific scopes) in the first argument? Or is it better to standardize that it's always just "Construct" (and instead, the constructor can narrow down functionality based on project types using ProjectType.ofProjectType(scope)? I think the latter might be better long term.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@mergify mergify bot added the contribution/core ⚙️ used by automation label Mar 8, 2022
@@ -0,0 +1,541 @@
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I generated this for fun, just to see what the construct tree looks like. The paths look a tad ugly because "/" occurs a lot in file paths, but isn't allowed in construct paths, so they just get translated to "--". I wonder if there's a way to make this more natural, or maybe have a specific "projen-specific" path instead with a different delimiting scheme, etc

Comment on lines +22 to +40
/**
* Finds a JSON file by name in the given scope.
* @param filePath The file path. If this path is relative, it will be resolved
* from the root of the nearest project.
*/
public static tryFindJsonFile(
scope: IConstruct,
filePath: string
): ObjectFile | undefined {
const isObjectFile = (c: IConstruct): c is ObjectFile =>
c instanceof ObjectFile;
const absolutePath = path.isAbsolute(filePath)
? filePath
: path.resolve(Project.ofProject(scope).outdir, filePath);
return scope.node
.findAll()
.filter(isObjectFile)
.find((file) => file.absolutePath === absolutePath);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By switching to constructs, escape hatching is more flexible since anyone can directly access construct.node. However I think these "tryFind" methods are still useful if you don't know where a file is generated from.

I'm not sure if this functionality should be on Project, or if it should be on the filetype classes instead. We could also just expose tryFind/tryFindFile in the Project class and just expose isXxx static methods on these classes instead.

(re: naming -- if we choose to make them static methods, we can't name them all tryFind because of aws/jsii#3410 so they have to be a little verbose)

@pgollucci
Copy link
Contributor

you probably want a pr/do-not-merge on this one

@Chriscbr
Copy link
Contributor Author

Closing for now as stale

@Chriscbr Chriscbr closed this Jun 13, 2022
@Chriscbr Chriscbr reopened this Jul 3, 2022
@mrgrain mrgrain mentioned this pull request Oct 9, 2023
14 tasks
@mrgrain mrgrain closed this Oct 26, 2023
@mrgrain
Copy link
Contributor

mrgrain commented Oct 26, 2023

Implemented by #2974

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core ⚙️ used by automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants