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

Make Plexus output ESM #1219

Merged
merged 1 commit into from
Feb 27, 2023
Merged

Conversation

mszabo-wikia
Copy link
Contributor

Which problem is this PR solving?

Split from #1212

Short description of the changes

Plexus currently outputs a Universal Module Definition, which seems to cause issues during the Vite build as it results in "ambiguous exports". Have Plexus output ESM instead to avoid the issue.

Plexus currently outputs a Universal Module Definition, which seems to
cause issues during the Vite build as it results in "ambiguous exports".
Have Plexus output ESM instead to avoid the issue.

Signed-off-by: Máté Szabó <mszabo@fandom.com>
@yurishkuro
Copy link
Member

what is the implication of this? My brief read-up indicates that UMD has broader compatibility. What are we losing by switching to ESM?

output: {
path: layoutDir,
publicPath: '/',
filename: '[name].bundled.js',
library: 'layout.worker.bundled',
Copy link
Member

Choose a reason for hiding this comment

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

I remember there was some weirdness with this layout.worker - did you try running the deep dependency graphs after this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I tested rendering a DDG from a proxied-in Jaeger deployment that already had one, and it seems to load and render okay.

@codecov
Copy link

codecov bot commented Feb 27, 2023

Codecov Report

Base: 95.53% // Head: 95.41% // Decreases project coverage by -0.12% ⚠️

Coverage data is based on head (7f32e20) compared to base (5cb39f9).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1219      +/-   ##
==========================================
- Coverage   95.53%   95.41%   -0.12%     
==========================================
  Files         243      243              
  Lines        7571     7571              
  Branches     1898     1898              
==========================================
- Hits         7233     7224       -9     
- Misses        331      340       +9     
  Partials        7        7              
Impacted Files Coverage Δ
...nents/TracePage/TraceTimelineViewer/SpanBarRow.tsx 94.44% <0.00%> (-5.56%) ⬇️
...mponents/TracePage/TraceStatistics/tableValues.tsx 96.89% <0.00%> (-2.42%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mszabo-wikia
Copy link
Contributor Author

@yurishkuro As far as I understand, the current build system will just consume this dependency via ESM and bundle/transform it as needed; same for the Vite production build. The main error this should fix is in the Vite dev run, where it seems that the ESM interop code that wraps the output in the UMD case isn't handled well and results in the above error.

@yurishkuro yurishkuro merged commit 2c16fa5 into jaegertracing:main Feb 27, 2023
Binrix pushed a commit to Binrix/jaeger-ui that referenced this pull request Apr 18, 2023
## Which problem is this PR solving?
Split from jaegertracing#1212

## Short description of the changes
Plexus currently outputs a Universal Module Definition, which seems to
cause issues during the Vite build as it results in "ambiguous exports".
Have Plexus output ESM instead to avoid the issue.

Signed-off-by: Máté Szabó <mszabo@fandom.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants