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

Merge ligatures addon into core repo #2847

Merged
merged 20 commits into from
May 3, 2020

Conversation

LabhanshAgrawal
Copy link
Contributor

@LabhanshAgrawal LabhanshAgrawal commented Apr 15, 2020

Fixes #2726
@Tyriar see if this seems ok (first time contributing to xterm)

@Tyriar
Copy link
Member

Tyriar commented Apr 15, 2020

Looks pretty good, tests even run and pass 😃

I'll give it a proper review as soon as I can

@LabhanshAgrawal LabhanshAgrawal force-pushed the ligatures-addon branch 2 times, most recently from 3562446 to feab7c2 Compare April 15, 2020 22:40
@LabhanshAgrawal
Copy link
Contributor Author

I was trying to add a webpack config similar to other addons, but getting errors for fs module which is used by font-finder
Is this addon currently supposed to run only on electron/node?
If yes, would it be ok to use target node or electron-renderer in webpack config?

@Tyriar
Copy link
Member

Tyriar commented Apr 16, 2020

Yes it can only be used in node/electron as it pulls fonts from disk.

@LabhanshAgrawal
Copy link
Contributor Author

added webpack config similar to other addons

@LabhanshAgrawal
Copy link
Contributor Author

@Tyriar the ci on windows is failing because of ../../node_modules/.bin/tsc type of commands. cmd is not able to use this path, other addons have similar commands though. Can you suggest any workaround for this.
Also, webpack is including the font-finder and font-ligatures dependencies in the bundle. Should I add them as external?

@Tyriar
Copy link
Member

Tyriar commented Apr 17, 2020

@LabhanshAgrawal that's odd, pretty sure the same thing is run for the other addons so I don't know what the problem could be. Unless the addons don't actually get built on other platform in which case we should fix that generally before merging. If that is the case could you create an issue?

@LabhanshAgrawal
Copy link
Contributor Author

@Tyriar While doing npm install at root, the other addons are skipped as they don't have any dependencies. Since the ligature addon depends on few things, it's build step is triggered and that's failing on windows because of the ../../.....tsc thing.
Same thing might happen with the webpack, but as the Release step in the ci runs on ubuntu, it works.
All the addons get built in the tsc -b ./tsconfig.all.json step though.
Directly doing tsc -p was working (feab7c2), I changed it to be same as other addons.
Also, this issue is not there if you use powershell instead of cmd on windows.

@LabhanshAgrawal LabhanshAgrawal force-pushed the ligatures-addon branch 4 times, most recently from d911848 to 14ad58f Compare April 22, 2020 04:22
@LabhanshAgrawal
Copy link
Contributor Author

@Tyriar please review.

@Tyriar Tyriar added this to the 4.6.0 milestone Apr 22, 2020
@LabhanshAgrawal LabhanshAgrawal force-pushed the ligatures-addon branch 2 times, most recently from 154794c to 5d4c4d7 Compare April 27, 2020 05:56
@LabhanshAgrawal
Copy link
Contributor Author

rebase and pushed

@Tyriar
Copy link
Member

Tyriar commented Apr 27, 2020

This week I'm focusing on testing VS Code, next week I'll be merging in xterm.js PRs and releasing 4.6. I expect this should be a pretty easy merge then 🙂

@Tyriar
Copy link
Member

Tyriar commented May 3, 2020

I notice the addon doesn't yet depend on the core codebase so breakages could still happen. We should write integration test(s) so breakages are detected early #2896

@Tyriar
Copy link
Member

Tyriar commented May 3, 2020

Had an idea which could maybe do away with a lot of the complexity and native access in this addon #2897

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Thanks for this! It should be much easier to maintain this now, especially after some integration tests are added #2896

@Tyriar Tyriar merged commit fbdb01d into xtermjs:master May 3, 2020
@Tyriar Tyriar mentioned this pull request May 3, 2020
@LabhanshAgrawal LabhanshAgrawal deleted the ligatures-addon branch May 4, 2020 04:22
@LabhanshAgrawal
Copy link
Contributor Author

Will you be moving the issues from the old repo? if that's possible.

@Tyriar
Copy link
Member

Tyriar commented May 4, 2020

Done thanks, and archived the old one.

@regisbsb
Copy link

vercel/hyper#3607

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.

Merge ligatures addon into core repo
3 participants