-
Notifications
You must be signed in to change notification settings - Fork 471
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
Change Vite config from CJS to ESM #1992
Change Vite config from CJS to ESM #1992
Conversation
This commit fixes the CJS deprication warning. Signed-off-by: shivam <[email protected]>
@@ -13,11 +13,11 @@ | |||
// limitations under the License. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- are the
*.mts
files included in our linter? - instead of renaming the file, why not add
"type": "module"
to./packages/jaeger-ui/package.json
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are the *.mts files included in our linter?
no they arent.. forgot to check that.
yeah you are right that would be better. making the change...
This commit fixes the CJS deprication warning Signed-off-by: shivam <[email protected]>
01e8836
to
5559770
Compare
hey @yurishkuro some tests are failing as they are using require() instead of import... i think this could be maybe resolved by using .cjs extension for tests in order to use require statements?? or we could rewrite the tests and use import statements instead... |
can you analyze how many changes would be needed? Fixing |
yup currently there are a total of 18 occurances of require in the project. changing around 11 of them located in the |
The previous commit changed the config from CJS to ESM which caused 2 tests to fail. This commit fixes the above issue by replacing 'require' with 'import' throughout the project Signed-off-by: shivam <[email protected]>
…aeger-ui into vite_ESM_upgrade
Head branch was pushed to by a user without write access
@yurishkuro made the changes...tests should be able to pass now |
you can speed up the iterations by running the tests locally ( |
This commit fixes the CJS deprication warning by renaming vite.config.ts to vite.config.mts. Signed-off-by: shivam <[email protected]>
hey @yurishkuro i think its best to rename vite.config to mts. changing imports to require is causing few tests to fail which required renaming a bunch of other files that uses CJS which would defeat the purpose of this issue. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1992 +/- ##
=======================================
Coverage 96.54% 96.54%
=======================================
Files 256 256
Lines 7604 7604
Branches 1978 1978
=======================================
Hits 7341 7341
Misses 263 263 ☔ View full report in Codecov by Sentry. |
Thanks @ShivamMadlani |
This commit fixes the CJS deprication warning.
Which problem is this PR solving?
Description of the changes
type: module
topackage.json
How was this change tested?
yarn test
Checklist
jaeger
:make lint test
jaeger-ui
:yarn lint
andyarn test