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.

Sign up for GitHub

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

Jump to bottom

Change imports for Node.js compatibility #4377

Merged
merged 7 commits into from
Aug 29, 2024
Merged

Change imports for Node.js compatibility #4377

merged 7 commits into from
Aug 29, 2024

Conversation

Johennes
Copy link
Contributor

@Johennes Johennes commented Aug 23, 2024 •
edited
Loading

Fixes: #4287

This is a variation of #4375 that uses .ts extensions and https://babeljs.io/docs/babel-preset-typescript#rewriteimportextensions.

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • Sign-off given on the changes (see CONTRIBUTING.md).

@github-actions github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Aug 23, 2024
@Johennes Johennes marked this pull request as ready for review August 23, 2024 11:25
@Johennes Johennes requested review from a team as code owners August 23, 2024 11:25
@Johennes Johennes mentioned this pull request Aug 23, 2024
Closed
@Johennes Johennes changed the title Change imports for Node.js compatibility (Next try) Change imports for Node.js compatibility (next try) Aug 23, 2024
@richvdh richvdh added the T-Task Tasks for the team like planning label Aug 23, 2024
@richvdh
Copy link
Member

richvdh commented Aug 23, 2024

🎉

richvdh
richvdh reviewed Aug 23, 2024
View reviewed changes
babel.config.cjs Outdated Show resolved Hide resolved
richvdh
richvdh approved these changes Aug 23, 2024
View reviewed changes
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@Johennes
Copy link
Contributor Author

For resolving conflicts from here on, it should only be a matter of running the script from #4375 again (after replacing js with ts). Happy to do it if you give me a ping whenever you'd like to land this.

@richvdh richvdh changed the title Change imports for Node.js compatibility (next try) Change imports for Node.js compatibility Aug 27, 2024
@richvdh
Copy link
Member

richvdh commented Aug 28, 2024

@matrix-org/element-call-reviewers : would really love a review on this please...

@richvdh richvdh added this pull request to the merge queue Aug 29, 2024
Merged via the queue into matrix-org:develop with commit 05ba27f Aug 29, 2024
23 checks passed
@florianduros
Copy link
Contributor

This is breaking the CI of the react sdk https://github.com/matrix-org/matrix-react-sdk/actions/runs/10627716730/job/29461292604?pr=12938

@richvdh
Copy link
Member

richvdh commented Aug 30, 2024

Sigh. Annoying that it didn't get picked up by the CI on this PR. I guess you need to set allowImportingTsExtensions in the react-sdk too, if you're going to continue to use the TS source rather than the transpiled js.

@florianduros
Copy link
Contributor

florianduros commented Aug 30, 2024 •
edited
Loading

Sadly it's not enough matrix-org/matrix-react-sdk#12939 I'm still investigating

This was referenced Aug 30, 2024
Merged
Merged
@Johennes
Copy link
Contributor Author

This is breaking the CI of the react sdk https://github.com/matrix-org/matrix-react-sdk/actions/runs/10627716730/job/29461292604?pr=12938

Ugh, sorry and thanks for fixing this @florianduros.

@richvdh richvdh mentioned this pull request Oct 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndrewFerr AndrewFerr AndrewFerr approved these changes

@richvdh richvdh richvdh approved these changes

@BillCarsonFr BillCarsonFr Awaiting requested review from BillCarsonFr BillCarsonFr is a code owner automatically assigned from matrix-org/element-crypto-web-reviewers

@robintown robintown Awaiting requested review from robintown robintown is a code owner automatically assigned from matrix-org/element-web-reviewers

Assignees
No one assigned
Labels
T-Task Tasks for the team like planning Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

matrix-js-sdk no longer works in node
4 participants
@Johennes @richvdh @florianduros @AndrewFerr