-
Notifications
You must be signed in to change notification settings - Fork 32
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
Migrate ace-diff from Webpack to Vite #1736
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1736 +/- ##
==========================================
- Coverage 76.80% 76.80% -0.01%
==========================================
Files 531 531
Lines 42258 42258
Branches 270 270
==========================================
- Hits 32458 32456 -2
- Misses 9800 9802 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
rollupOptions: { | ||
input: ['./src/index.js', './src/styles/ace-diff.scss', './src/styles/ace-diff-dark.scss'], | ||
output: { | ||
format: 'commonjs', |
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.
We don't need commonjs output. Should this be changed?
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.
I can probably re-work it to be ESM, but it was commonjs before, so I was just doing the minimal change to get ace-diff on vite
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.
Actually yeah, it looks like it'll probably be happy as ESM, which is what Vite wants anyway, so I can undo the changes to admin's vite config
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.
I think umd is probably the correct option for now? Making this esm I don't think would hurt since this one is being bundled, but for tool-common I don't think we want esm yet. From what I read commonjs is only for node backends.
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.
For linked/bundled deps (which I think is only ace-diff), I'd rather use ESM since it's the default for Vite.
tool-common should be systemjs since that's how we're loading it in the app, and then we can build vue-common and js-common out to multiple formats (including systemjs) for publishing in #1737
Quality Gate passedIssues Measures |
No description provided.