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

Transpile @vector-im/compound-web with babel #25906

Merged
merged 3 commits into from
Aug 2, 2023

Conversation

germain-gg
Copy link
Contributor

@germain-gg germain-gg commented Aug 2, 2023

Fixes #25905
For #25883

To review alongside matrix-org/matrix-react-sdk#11354

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

This change is marked as an internal change (Task), so will not be included in the changelog.

@germain-gg germain-gg added the T-Task Tasks for the team like planning label Aug 2, 2023
@germain-gg germain-gg requested a review from a team as a code owner August 2, 2023 09:23
@germain-gg germain-gg requested review from dbkr and t3chguy August 2, 2023 09:23
@t3chguy
Copy link
Member

t3chguy commented Aug 2, 2023

@germain-gg could you include the rationale for this in the PR description? Why not just consume the built artifacts from the library like we do for the vast majority of our dependencies (both internal & external)

@germain-gg
Copy link
Contributor Author

@t3chguy we essentially need it because it otherwise throws

2023-08-02 10:29:36.683 [element-js] ERROR in ../matrix-react-sdk/node_modules/@vector-im/compound-web/dist/compound-web.js 1482:22
2023-08-02 10:29:36.683 [element-js] Module parse failed: Unexpected token (1482:22)
2023-08-02 10:29:36.683 [element-js] You may need an appropriate loader to handle this file type, currently no loaders are configured to process this file. See https://webpack.js.org/concepts#loaders
2023-08-02 10:29:36.683 [element-js] |         onMouseDown: (e) => {
2023-08-02 10:29:36.683 [element-js] |           e.preventDefault();
2023-08-02 10:29:36.683 [element-js] >           onMouseDown?.(e);
2023-08-02 10:29:36.683 [element-js] |         }
2023-08-02 10:29:36.683 [element-js] |       },

We still use the ?. optional chaining operator in the original bundle because our browser support does not require us to transpile it, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining#browser_compatibility

But our webpack/babel setup is tricking us in having to do it anyway. And i'm not looking to fix this at this stage.

// either webpack or our babel setup.
// When we do get to upgrade our current setup, this should
// probably be removed.
if (f.includes("compound-web")) return true;
Copy link
Member

Choose a reason for hiding this comment

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

Could we be more specific, there might be transitive imports which also contain the text compound-web

@germain-gg germain-gg requested a review from t3chguy August 2, 2023 09:45
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

LGTM

@germain-gg germain-gg enabled auto-merge (squash) August 2, 2023 10:02
@germain-gg germain-gg merged commit c6756ea into develop Aug 2, 2023
18 checks passed
@germain-gg germain-gg deleted the germain-gg/compound-web-integration branch August 2, 2023 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Task Tasks for the team like planning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate @vector-im/compound-web
2 participants