Skip to content
This repository has been archived by the owner on Jan 24, 2023. It is now read-only.

Aot #2594

Merged
merged 14 commits into from Jul 19, 2018
Merged

Aot #2594

merged 14 commits into from Jul 19, 2018

Conversation

KlapTrap
Copy link
Contributor

@KlapTrap KlapTrap commented Jun 29, 2018

Fix up the app to allow for our app to be built with aot enabled.

Fixes #2201

@KlapTrap KlapTrap self-assigned this Jun 29, 2018
@cfdreddbot
Copy link

Hey KlapTrap!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

* v2-master: (31 commits)
  RC2 Changelog
  Apply `connected` fix to marketplace wall as well
  Filter for connected endpoints
  Remove testing code
  Clear up service instances pagination after disconnecting an endpoint
  Fix bug in reducer
  Change schedular of application step 1 validated observable to avoid zone issues
  Revert "Horrible work around for ExpressionChangedAfterItHasBeenCheckedError"
  Fix typo in button id
  Horrible work around for ExpressionChangedAfterItHasBeenCheckedError - We've got the `valid` pattern wrong in the steppers, the steps are child   components and change the state of their parent components - Added this workaround, but we should think about changing this pattern   (not sure how though, given that each step controls it's own valid state) - Have also tested other steppers for the same issue and this looks like   the only one
  Fix e2e-tests
  Fix indentation
  Add e2e-tests branch
  Fix artifact upload
  Fix missing package. Only run e2e tests for testing
  Add screenshot reporter for test failures. Upload e2e test report to s3.
  Test
  Try fix for slower test execution
  Fix create app step 'valid' which disabled stepper next in production - In production the statusChanges emits before we watch it for the result - when we start watching it doesn't emit the now valid form state - only seen in production when navigating to stepper, not when loading stepper
  Fixed space delete and issue with entity service
  ...
@codecov
Copy link

codecov bot commented Jun 29, 2018

Codecov Report

Merging #2594 into v2-master will decrease coverage by 0.04%.
The diff coverage is 81.35%.

@@              Coverage Diff              @@
##           v2-master    #2594      +/-   ##
=============================================
- Coverage       70.1%   70.06%   -0.05%     
=============================================
  Files            599      599              
  Lines          25515    25482      -33     
  Branches        5783     5757      -26     
=============================================
- Hits           17888    17853      -35     
- Misses          7627     7629       +2

Copy link
Contributor

@richard-cox richard-cox left a comment

Choose a reason for hiding this comment

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

Gone through code and had some questions/issues. Struggling to get it to run locally though.

@@ -1 +1 @@
<mat-radio-button [disabled]="(dataSource.isAdding$ | async) || disable" (change)="dataSource.selectedRowToggle(row, false)" [checked]="dataSource.selectedRows?.has(dataSource.getRowUniqueId(row))"></mat-radio-button>
<mat-radio-button [disabled]="(dataSource.isAdding$ | async) || disable" (change)="dataSource.selectedRowToggle(row)" [checked]="dataSource.selectedRows?.has(dataSource.getRowUniqueId(row))"></mat-radio-button>
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a mistake in the interface for selectedRowToggle, it should have an optional second parameter that defaults to true (see list.data.source).

) {
// Note - normal optional parameter notation won't work with injectable
this.selectMode = _selectMode || this.selectMode;
this.selectMode = this.selectMode;
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're not setting this via constructor it doesn't need to be set here.


@Component({
selector: 'app-cards',
templateUrl: './cards.component.html',
styleUrls: ['./cards.component.scss']
})
export class CardsComponent<T> {
public columns = CardCell.columns;
Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks CfEndpointCardComponent which overrides the static component.columns/CardCell.columns
Update - this is ok with latest solution

Copy link
Contributor

@richard-cox richard-cox left a comment

Choose a reason for hiding this comment

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

After adding a custom start target this worked fine for me.
From a purely 'does it work' perspective the PR looks fine.

@KlapTrap
Copy link
Contributor Author

We need another npm target that builds the app without aot and @richard-cox was going to add a target that will run the dev build with an increased memory limit. We might want to document those new targets too.

nwmac and others added 2 commits July 19, 2018 10:16
* v2-master: (101 commits)
  More tidy up
  Tidied up some code
  linting fix
  Tweaks as per discussion, also upped page size from 50 to 100 for permissions fetch
  Fix tests
  Linting
  Flatten pagination for current user permission fetching
  Ensure lists with no entities populate pagination section correctly - ids: { } => ids: { 1: [] } - think this is a regression following the removal of partial entities - Fixes #2682, #2684
  Make flattenPagination generic
  Add note about memory usage
  Clean up whitespace
  Ensure we don't worry about empty page request infos if we already have is list
  Fix for add org button vanishing
  Fix unit tests
  Fix cc
  Improve styling of new chips button
  Fix click action interferring with scroll bar
  Minor tidy ups
  Use subtle mode by default for the boolean indicator
  Add support for new SCF Config LB property
  ...
@KlapTrap KlapTrap merged commit 4bf1710 into v2-master Jul 19, 2018
@KlapTrap KlapTrap deleted the aot branch July 19, 2018 10:14
irfanhabib pushed a commit that referenced this pull request Jul 19, 2018
Fix up the app to allow for our app to be built with aot enabled.

Fixes #2201
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs attention This PR needs attention
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants