Skip to content

Conversation

@LeeLenaleee
Copy link
Collaborator

TODO:

Add note in migration guide after #10010 is merged that has setup for migration guide

@LeeLenaleee LeeLenaleee added this to the Version 4.0 milestone Aug 2, 2022
@LeeLenaleee
Copy link
Collaborator Author

LeeLenaleee commented Aug 2, 2022

Seems package-lock.json has been (partially) removed, will fix
Fixed

etimberg
etimberg previously approved these changes Aug 2, 2022
Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

Needs the migration notes

kurkle
kurkle previously approved these changes Aug 5, 2022
etimberg
etimberg previously approved these changes Aug 5, 2022
@etimberg
Copy link
Member

etimberg commented Aug 5, 2022

Migration guide needs a rebase it seems

@LeeLenaleee LeeLenaleee dismissed stale reviews from etimberg and kurkle via fe031db August 5, 2022 12:28
etimberg
etimberg previously approved these changes Aug 5, 2022
Comment on lines +5 to +7
import { readFileSync } from "fs";

const {version, homepage} = JSON.parse(readFileSync('./package.json'));
Copy link
Collaborator Author

@LeeLenaleee LeeLenaleee Aug 5, 2022

Choose a reason for hiding this comment

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

Can be changed to:

import * as packageJson from './package.json' assert {type: "json"};

But then the rollup file needs to be excluded from eslint, not sure what the better/nicer solution is

Might be fixable with extra eslint config: eslint/eslint#15305 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this way is just fine. Import assertions aren't standardized yet and are still at the proposal phase

Copy link
Contributor

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

thanks for fixing the failing build for me

@kurkle kurkle merged commit 3df687a into chartjs:master Aug 5, 2022
@dangreen
Copy link
Collaborator

dangreen commented Aug 5, 2022

@kurkle @LeeLenaleee fyi

Chart.js(feat-side-effects-false)$ npx eslint karma.conf.cjs 

/Users/dangreen/github/Chart.js/karma.conf.cjs
  9:15  error  Multiple spaces found before '='       no-multi-spaces
  9:26  error  ES2020 'import()' syntax is forbidden  es/no-dynamic-import

✖ 2 problems (2 errors, 0 warnings)
  1 error and 0 warnings potentially fixable with the `--fix` option.

@LeeLenaleee
Copy link
Collaborator Author

Interesting ci did not fail on it, will look at it later

@LeeLenaleee
Copy link
Collaborator Author

LeeLenaleee commented Aug 5, 2022

@dangreen I don't seen the get the error when runnning npm run lint-js so I guess your way of running eslint might take another config file.

EDIT:
I don't know why why eslint does not lint the file because it should indeed flag the double space with the normal way of running it, guess it's fine for now since it does not trigger with the npm scripts , if it does we need to adjust the eslint config to either allow dynamic imports or disable linting of the karma configuration

@LeeLenaleee LeeLenaleee deleted the remove-destroy-hook branch August 5, 2022 18:46
@dangreen
Copy link
Collaborator

dangreen commented Aug 5, 2022

@LeeLenaleee run directly npx eslint karma.conf.cjs

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.

5 participants