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

Use strictTemplates Compiler Option #100

Merged
merged 34 commits into from
Jun 11, 2020

Conversation

tobiasschweizer
Copy link
Contributor

@tobiasschweizer tobiasschweizer commented Jun 9, 2020

resolves https://dasch.myjetbrains.com/youtrack/issue/DSP-319

requires #101

This PR makes the lib compile with Ivy using "strictTemplates": true. Compilation problems are ignored using $any() (https://angular.io/guide/template-syntax#the-any-type-cast-function).
However, the following issues still have to be fixed:

This PR splits tests into groups:

  • unit tests and productive build of lib and app (required)
  • dev build of lib
  • E2E tests (required)

I did not make dev build of lib required since we do not use it in production (Ivy is not used to build the lib for production). However, this test should pass. More checks by the compiler should lead to fewer problems at runtime, that's my rationale.

@tobiasschweizer tobiasschweizer added enhancement New feature or request build labels Jun 9, 2020
@tobiasschweizer tobiasschweizer self-assigned this Jun 9, 2020
@tobiasschweizer
Copy link
Contributor Author

@mdelez I am going to group tests on this PR, so tests are run individually (like in https://github.com/dasch-swiss/knora-api/blob/f984c2e0589aec81e28c45adde4ff76d9820b640/.github/workflows/main.yml#L62-L142). And not all of them would be required for merging, so we could detect problems without blocking development.


strategy:
matrix:
node-version: [12.x]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@subotic Do you happen to know if there could be one central version matrix? This would be easier to update.

Copy link

Choose a reason for hiding this comment

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

You could try and see if defaults could allow you to define a value which you then could use in all the different matrix statements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do, thx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@subotic Does not seem to be possible with "defaults:"

Screenshot 2020-06-10 at 14 19 09

Screenshot 2020-06-10 at 14 19 20

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or did I do it wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I am giving up on that ...

@tobiasschweizer
Copy link
Contributor Author

@mdelez I made different groups of tests, and not all of them are required.

@mdelez
Copy link
Contributor

mdelez commented Jun 10, 2020

@mdelez I made different groups of tests, and not all of them are required.

Perfect!

<dsp-time-value class="parent-value-component" #displayVal *ngSwitchCase="constants.TimeValue" [mode]="mode" [displayValue]="displayValue"></dsp-time-value>
<dsp-geoname-value class="parent-value-component" #displayVal *ngSwitchCase="constants.GeonameValue" [mode]="mode" [displayValue]="displayValue"></dsp-geoname-value>
<dsp-link-value class="parent-value-component" #displayVal *ngSwitchCase="constants.LinkValue" [mode]="mode" [displayValue]="displayValue"
<dsp-text-value-as-string class="parent-value-component" #displayVal *ngSwitchCase="'ReadTextValueAsString'" [mode]="mode" [displayValue]="$any(displayValue)"></dsp-text-value-as-string>
Copy link
Contributor

Choose a reason for hiding this comment

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

should I work on making a service that returns the correct value type? It shouldn't take too long and then you can use the service in this branch so we don't have to use temporary code that may get left in the master branch for a while

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 think the other two issues are more important. Could we do them first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case of the DisplayEditComponent, we know that this will be correct at runtime. We just have to tell the compiler somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mdelez Now that solved all the other problems, you can work on this :-)

@tobiasschweizer
Copy link
Contributor Author

not to myself: make e2e tests required when this PR has been merged.

@tobiasschweizer
Copy link
Contributor Author

@kilchenmann @mdelez @flavens Could you have a look at this PR? I'd like to merge it before working on migrating the advanced search, so that the new code is compiled in strict template mode.

@tobiasschweizer
Copy link
Contributor Author

@mdelez I think it is ok to use $any() in DisplayEditCompopnent's template for the moment because we know the correct type will be passed at runtime.

@mdelez mdelez self-requested a review June 11, 2020 07:16
@tobiasschweizer tobiasschweizer merged commit 79741a7 into master Jun 11, 2020
@tobiasschweizer tobiasschweizer deleted the wip/DSP-319-strict-templates-compilation branch June 11, 2020 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants