Skip to content

Conversation

@vursen
Copy link
Contributor

@vursen vursen commented Jun 10, 2021

Description

Since the Template API has been removed, we need to warn the user to import @vaadin/vaadin-template-renderer if he attempts to use templates inside a component that previously supported the API.

  • vaadin-combo-box
  • vaadin-notification
  • vaadin-select
  • vaadin-context-menu
  • vaadin-dialog
  • vaadin-grid
  • vaadin-grid-pro
  • vaadin-virtual-list

Fixes #312, #1970.

Type of change

  • Feature

Checklist

  • I have read the contribution guide: https://vaadin.com/docs-beta/latest/guide/contributing/overview/
  • I have added a description following the guideline.
  • The issue is created in the corresponding repository and I have referenced it.
  • I have added tests to ensure my change is effective and works as intended.
  • New and existing tests are passing locally with my change.
  • I have performed self-review and corrected misspellings.

Additional for Feature type of change

  • Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

@vursen vursen added the no-polymer Removing Polymer from Vaadin public APIs label Jun 10, 2021
@vursen vursen self-assigned this Jun 10, 2021
@vursen vursen requested a review from tomivirkki June 10, 2021 09:56
};
});

it('should remove template when added after renderer', () => {
Copy link
Contributor Author

@vursen vursen Jun 10, 2021

Choose a reason for hiding this comment

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

It is a no longer relevant test since we've made the template renderer responsible for throwing such exceptions.

I missed this test while removing the Template API as far as it successfully passes by the reason of another exception (notification._observer.flush() throws an exception as we don't have _observer defined anymore).

@vursen vursen force-pushed the feat/template-api-warning branch 3 times, most recently from 069eecc to d6ce1cc Compare June 11, 2021 06:44
@vursen vursen force-pushed the feat/template-api-warning branch from d6ce1cc to 793b80d Compare June 11, 2021 09:40
import sinon from 'sinon';
import { GridElement } from '@vaadin/vaadin-grid/src/vaadin-grid.js';
import { GridColumnElement } from '@vaadin/vaadin-grid/src/vaadin-grid-column.js';
import '@vaadin/vaadin-template-renderer';
Copy link
Contributor Author

@vursen vursen Jun 11, 2021

Choose a reason for hiding this comment

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

I forgot to import the template renderer here while removing the Template API as all the test cases in this file successfully pass regardless of a provided template (reason: tests don't actually use this template).

Missing has become noticeable since we are showing the warning:

packages/vaadin-grid-pro/test/grid-pro.test.js:

 🚧 Browser logs:
      WARNING: <template> inside <vaadin-grid-pro> is no longer supported. Import @vaadin/vaadin-template-renderer to enable compatibility (see https://vaad.in/template-renderer)
      WARNING: <template> inside <vaadin-grid-column> is no longer supported. Import @vaadin/vaadin-template-renderer to enable compatibility (see https://vaad.in/template-renderer)
      WARNING: <template> inside <vaadin-grid-pro> is no longer supported. Import @vaadin/vaadin-template-renderer to enable compatibility (see https://vaad.in/template-renderer)
      WARNING: <template> inside <vaadin-grid-column> is no longer supported. Import @vaadin/vaadin-template-renderer to enable compatibility (see https://vaad.in/template-renderer)
      WARNING: <template> inside <vaadin-grid-pro> is no longer supported. Import @vaadin/vaadin-template-renderer to enable compatibility (see https://vaad.in/template-renderer)
      WARNING: <template> inside <vaadin-grid-column> is no longer supported. Import @vaadin/vaadin-template-renderer to enable compatibility (see https://vaad.in/template-renderer)
      WARNING: <template> inside <vaadin-grid-pro> is no longer supported. Import @vaadin/vaadin-template-renderer to enable compatibility (see https://vaad.in/template-renderer)
      WARNING: <template> inside <vaadin-grid-column> is no longer supported. Import @vaadin/vaadin-template-renderer to enable compatibility (see https://vaad.in/template-renderer)

</style>
<vaadin-combo-box-light
allow-custom-value=""
item-label-path="value"
Copy link
Contributor Author

@vursen vursen Jun 11, 2021

Choose a reason for hiding this comment

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

Removing this property doesn't change anything as by default item-label-path === "label" (proof) and item.value === item.label (proof).

dir="ltr"
theme$="[[theme]]"
>
<template> [[item.label]] </template>
Copy link
Contributor Author

@vursen vursen Jun 11, 2021

Choose a reason for hiding this comment

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

This template is not necessary here as by default the combo-box already renders item.label without any templates. As far as this template is not actually used, it can be removed.

@vursen vursen marked this pull request as ready for review June 11, 2021 11:46
@vursen vursen changed the title feat: warn when using legacy Template API feat: warn if using legacy Template API Jun 11, 2021
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@vursen vursen merged commit cad6f1a into master Jun 11, 2021
@vursen vursen deleted the feat/template-api-warning branch June 11, 2021 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-polymer Removing Polymer from Vaadin public APIs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

add a deprecation warning for using <template> inside Vaadin components

3 participants