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

refactor!: move text-field to new base class #2620

Merged
merged 8 commits into from
Sep 30, 2021

Conversation

web-padawan
Copy link
Member

@web-padawan web-padawan commented Sep 24, 2021

Description

Fixes #2220

Type of change

  • Breaking change

@web-padawan web-padawan added the a11y Accessibility issue label Sep 24, 2021
Copy link
Member

@tomivirkki tomivirkki left a comment

Choose a reason for hiding this comment

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

Needs an update to readme, otherwise lgtm

@web-padawan web-padawan removed the request for review from vursen September 25, 2021 10:06
@web-padawan
Copy link
Member Author

web-padawan commented Sep 25, 2021

There is a failure in Firefox which can be also reproduced locally with out of memory error:

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

 🚧 Browser logs:
      out of memory
      out of memory
      out of memory

 ❌ Error: done() called multiple times in hook <templates formatting "before each" hook in "formatting">; in addition, done() received error: Error: uncaught exception: out of memory (http://localhost:8000/packages/component-base/src/debounce.js:156) 

Example: https://github.com/vaadin/web-components/pull/2620/checks?check_run_id=3707687559

Related tests were introduced by vaadin/vaadin-grid@7a07299
But then that code was rewritten: vaadin/vaadin-grid@ba7e90d

I have no idea if the tests still make sense. But apparently something is wrong with the new text-field.
It could be related to usage of slots. I'm going to verify which mixins contain problematic code.

UPD: when updating vaadin-text-field to only include InputConstraintsMixin and InputSlotMixin the error doesn't happen. I will continue exploring which mixins might be causing this on Monday.

@tomivirkki
Copy link
Member

tomivirkki commented Sep 27, 2021

The memory issue can be reproduced with the following test file:

import { fixtureSync } from '@vaadin/testing-helpers';
import '@vaadin/text-field/vaadin-text-field.js';

for (let i = 0; i < 50; i++) {
  describe('text-fields', () => {
    it('should not run out of memory', () => {
      fixtureSync('<vaadin-text-field></vaadin-text-field>'.repeat(10));
    });
  });
}

@web-padawan web-padawan marked this pull request as draft September 29, 2021 12:52
@web-padawan web-padawan marked this pull request as ready for review September 30, 2021 07:06
@web-padawan web-padawan removed the request for review from tomivirkki September 30, 2021 07:12
@sonarcloud
Copy link

sonarcloud bot commented Sep 30, 2021

Kudos, SonarCloud Quality Gate passed!    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

@web-padawan web-padawan merged commit 2e2ff00 into master Sep 30, 2021
@web-padawan web-padawan deleted the refactor/text-field-move branch September 30, 2021 07:18
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with platform 22.0.0.alpha7 and is also targeting the upcoming stable 22.0.0 version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate vaadin-text-field to use new base class
3 participants