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

Change Vite config from CJS to ESM #1992

Merged
merged 8 commits into from
Nov 25, 2023

Conversation

ShivamMadlani
Copy link
Contributor

@ShivamMadlani ShivamMadlani commented Nov 21, 2023

This commit fixes the CJS deprication warning.

Which problem is this PR solving?

Description of the changes

  • Add type: module to package.json
  • Imported visualizer as a function and used as a PluginOption

How was this change tested?

  • yarn test

Checklist

This commit fixes the CJS deprication warning.

Signed-off-by: shivam <shivammadlani5@gmail.com>
@ShivamMadlani ShivamMadlani requested a review from a team as a code owner November 21, 2023 21:18
@ShivamMadlani ShivamMadlani requested review from yurishkuro and removed request for a team November 21, 2023 21:18
@@ -13,11 +13,11 @@
// limitations under the License.
Copy link
Member

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?

Copy link
Contributor Author

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 <shivammadlani5@gmail.com>
@yurishkuro yurishkuro changed the title Resolves #1985 Change Vite config from CJS to ESM Nov 22, 2023
@yurishkuro yurishkuro added the changelog:ci Change related to continuous integration / testing label Nov 22, 2023
yurishkuro
yurishkuro previously approved these changes Nov 22, 2023
@yurishkuro yurishkuro enabled auto-merge (squash) November 22, 2023 15:47
@ShivamMadlani
Copy link
Contributor Author

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...

@yurishkuro
Copy link
Member

can you analyze how many changes would be needed? Fixing require would be my preferred path, but if it's too much work then perhaps just renaming vite config to .mts is a better fix (we'd need to include it in the linter globs)

@ShivamMadlani
Copy link
Contributor Author

yup currently there are a total of 18 occurances of require in the project. changing around 11 of them located in the test folder should pass all the tests. however we could just change all the requires so the whole project gets cleaned up...waiting for your thumbsup

ShivamMadlani and others added 3 commits November 23, 2023 12:59
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 <shivammadlani5@gmail.com>
auto-merge was automatically disabled November 23, 2023 07:36

Head branch was pushed to by a user without write access

@ShivamMadlani
Copy link
Contributor Author

@yurishkuro made the changes...tests should be able to pass now

@yurishkuro
Copy link
Member

you can speed up the iterations by running the tests locally (yarn lint and yarn test) instead of waiting for me to approve the CI run

This commit fixes the CJS deprication warning by renaming vite.config.ts
to vite.config.mts.

Signed-off-by: shivam <shivammadlani5@gmail.com>
@ShivamMadlani
Copy link
Contributor Author

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.
however i also added .mts to our formatter and yarn tests are passing locally.

Copy link

codecov bot commented Nov 25, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (72106e4) 96.54% compared to head (75ad7d4) 96.54%.

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.
📢 Have feedback on the report? Share it here.

@yurishkuro yurishkuro enabled auto-merge (squash) November 25, 2023 15:21
@yurishkuro yurishkuro merged commit 4ac86ef into jaegertracing:main Nov 25, 2023
10 checks passed
@ShivamMadlani ShivamMadlani deleted the vite_ESM_upgrade branch November 25, 2023 15:50
@yurishkuro
Copy link
Member

Thanks @ShivamMadlani

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:ci Change related to continuous integration / testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to ESM build of Vite
2 participants