-
Notifications
You must be signed in to change notification settings - Fork 1
Conversation
@ink404 @bradymadden97 tagging you here to review if nothing got lost along the way (I'm terrible at |
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.
Was adding this intentional?
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.
this was introduced in here #5 CC @bradymadden97
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.
Ah awesome, missed that PR a while back
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.
These were probably generated by the ROR github import 🤷
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.
+1 are we publishing this as a nix package? it was previously only published to be used as an npm dep
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.
no, just npm, again comes from that PR #5
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.
lgtm, thank you @szymonkaliski!
Would hold off for Brady to chick his changes though
Yes, definitely; tbh. I'm not sure what merging this will do -- roll back |
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.
LGTM, I released 5.3.0 yesterday though, is this just catching github up to what's released? Or are there new changes?
If new changes, we need to build as 5.3.1 and then release again.
In general I think we should probably merge to github main first then release
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.
These were probably generated by the ROR github import 🤷
@bradymadden97 I tried explaining the issues in the PR description, mainly this:
|
This branch is checked out at a commit that was made with
5.3.0
release on officialxterm.js
repo: xtermjs@2e02c37This branch also contains all of our patches.
The release process is as follows:
use node 18
run:
and finally
npm login && npm publish
We originally published
@replit/xterm.js
version5.3.0
frommaster
which is a bunch of commits ahead of this one, and introduces a bug with unicode. I unpublished that version, and will re-publish this as5.3.0
on Monday (need to wait 24h fornpm
to allow us to publish the same package version again, for whatever reason).EDIT: after testing, I also had to pull in xtermjs#4785 as it fixes an issue with
Cannot read properties of undefined (reading 'onRequestRedraw')
which we started getting when disposing shell/console. This patch will land with5.4.0
which is in beta right now.I also decided to call our version
{x}.{y}.{z}-patched.{num}
so it's bit clearer what we're doing, and we can keep updating thenum
without having to bump minor/patch number.