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: rewrite config helpers to ts #11121

Merged
merged 2 commits into from
Mar 21, 2023
Merged

refactor: rewrite config helpers to ts #11121

merged 2 commits into from
Mar 21, 2023

Conversation

dangreen
Copy link
Collaborator

@dangreen dangreen commented Feb 6, 2023

No description provided.

@etimberg etimberg added this to the Version 4.3.0 milestone Feb 7, 2023
etimberg
etimberg previously approved these changes Feb 7, 2023
@LeeLenaleee LeeLenaleee added type: enhancement type: types Typescript type changes labels Feb 10, 2023
LeeLenaleee
LeeLenaleee previously approved these changes Feb 10, 2023
@etimberg
Copy link
Member

@dangreen looks like some conflicts to resolve before this can be merged

@stockiNail
Copy link
Contributor

@etimberg @dangreen apologize if I'm jumping in this thread but probably it would be better to evaluate pending PRs on the touched files before translating in TS. See PR #11105.

@etimberg
Copy link
Member

@stockiNail I can go either way depending on when this gets rebased. I don't want this PR to have get rebased many times since that's a lot of work

@dangreen dangreen dismissed stale reviews from LeeLenaleee and etimberg via ad1cff0 February 20, 2023 11:37
etimberg
etimberg previously approved these changes Feb 20, 2023
LeeLenaleee
LeeLenaleee previously approved these changes Feb 20, 2023
@LeeLenaleee
Copy link
Collaborator

Could you rebase master into this so the CI works again?

@dangreen dangreen dismissed stale reviews from LeeLenaleee and etimberg via c0448c2 February 23, 2023 13:07
etimberg
etimberg previously approved these changes Feb 23, 2023
@LeeLenaleee
Copy link
Collaborator

Test seem to be failing

etimberg
etimberg previously approved these changes Feb 23, 2023
@dangreen
Copy link
Collaborator Author

@LeeLenaleee I've got randomly failing tests that are passing successfully locally.

@stockiNail
Copy link
Contributor

@LeeLenaleee I've got randomly failing tests that are passing successfully locally.

@LeeLenaleee @dangreen I think the issue is related to FF 110. I have the same issue in chartjs-plugin-annotation. See chartjs/chartjs-plugin-annotation#856 (comment)

If you see the history of the CI, you can see that the last one which ended correctly (testing the cases) was using FF 109. Starting using 110, the fixture test cases are failing randomly. And as @dangreen reported, locally it works correctly.

@stockiNail
Copy link
Contributor

stockiNail commented Feb 25, 2023

FYI, I had a look to new feature of FF 110: https://www.mozilla.org/en-US/firefox/110.0/releasenotes/
I see the following feature which could affect us: GPU-accelerated Canvas2D is enabled by default on macOS and Linux.

Therefore in the test PR in annotation plugin, I have tried to disable it (not sure if the property is the correct one, :() with the following Karma config:

    customLaunchers: {
      chrome: {
        base: 'Chrome',
        flags: [
          '--disable-accelerated-2d-canvas',
          '--disable-background-timer-throttling',
          '--disable-backgrounding-occluded-windows',
          '--disable-renderer-backgrounding'
        ]
      },
      firefox: {
        base: 'Firefox',
        prefs: {
          'layers.acceleration.disabled': true,
          'gfx.canvas.accelerated': false   // <--- added this one!!! --->
        }
      }
    },

and CI in the plugin ended correctly.

EDIT: I have submitted a PR chartjs/chartjs-plugin-annotation#857 in chartjs-plugin-annotation using the above configuration. Before applying here, I would wait for the results in the plugin. Anyway, I hope that 'gfx.canvas.accelerated' is the right property to disable (I haven't seen anyone else close to the feature meaning).

EDIT 2: merged in the master of plugin and sounds working. Just 1 side effect that I have seen: sometimes the test could fail due to the timeout. Re-running it, usually ends correctly.

@stockiNail
Copy link
Contributor

Submitted PR to fix FF 110 acceleration: #11165

@etimberg etimberg merged commit c0bf05f into chartjs:master Mar 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement type: types Typescript type changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants