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

Code Duplication #202

Open
colesanders opened this issue Nov 6, 2021 · 3 comments
Open

Code Duplication #202

colesanders opened this issue Nov 6, 2021 · 3 comments
Labels
approved Approved for Implementation enhancement New feature or request proposal Proposal for New Feature/Functionality

Comments

@colesanders
Copy link
Contributor

When extending the base facade you're required to provide a constructor along the lines of:

export EntityFacade extends EntityFacadeBase {
    constructor(private store: Store) {
        super(Entity, store);
    }
}

However, as the base facade comes from the buildState() call, it seems to be a pointless extra. The entity could instead be provided to the facade through the buildFacade() factory.

@Wells-Codes
Copy link
Contributor

@colesanders I think you have a good point and I like this idea. I'm pretty sure it could be accomplished easily (see a2772e0), but I don't think we'd be able to maintain backward compatibility. It'd break any apps using the old syntax (see 72c0a91)

I didn't thoroughly test that branch, because I'm guessing we won't move forward due to wanting to maintain backward compatibility. Maybe @jrista can weigh in

@jrista
Copy link
Contributor

jrista commented Nov 29, 2021

@Wells-Codes I chatted with @colesanders about it a bit. I am concerned about backwards compatibility, however we may be able to accomplish it with overloaded constructors. I haven't tried that yet, so we will have to see if it works.

@jrista
Copy link
Contributor

jrista commented Feb 19, 2022

It looks like there is no way to do overloaded constructors with TS. It would be nice to make this change, and it looks like @Wells-Codes started a branch for it. It might be best to time this change when we may have to make other breaking changes, which should happen soon when we support Angular 13 (and probably 14). We will have to make breaking changes at that point, and I think we could make this change then.

@jrista jrista added enhancement New feature or request proposal Proposal for New Feature/Functionality under review Currently under merit/design review labels Feb 19, 2022
@jrista jrista added approved Approved for Implementation and removed under review Currently under merit/design review labels Feb 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved for Implementation enhancement New feature or request proposal Proposal for New Feature/Functionality
Projects
None yet
Development

No branches or pull requests

3 participants