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

Automate Generation of d.ts for Ace Editor Modes #5518

Merged
merged 4 commits into from
Apr 5, 2024

Conversation

mkslanc
Copy link
Contributor

@mkslanc mkslanc commented Mar 21, 2024

Issue #, if available: #5511

Description of changes:
This PR introduces an automated process for generating TypeScript declaration files for the ACE Editor modes located within our src/mode directory. By analyzing our JavaScript codebase using the TypeScript Compiler API, we can now dynamically create a single ace-modes.d.ts file that accurately represents the types and exports of our editor modes.

This can serve as a short- to mid-term solution until the modes are converted to ES6 and properly checked with TypeScript.

Discussion Points:

  • Integration Timing: The optimal timing for integrating this script into our development or build process remains open for discussion. Input is needed to determine whether it should be run as part of our regular build process, pre-commit hooks, or at another stage to best suit our workflow.
  • Feedback and Improvements: Feedback on the accuracy of the generated types and suggestions for enhancing the script's integration with TypeScript projects are highly welcomed. Collaboration will be key in refining this process to fit our project needs seamlessly.

Next Steps:

I invite all ACE team members to review the proposed changes and participate in a discussion about integrating this script into workflow. Your insights are invaluable to ensuring the effectiveness and efficiency of this solution.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Pull Request Checklist:

Copy link

One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR.

1 similar comment
Copy link

One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR.

Copy link

codecov bot commented Mar 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.60%. Comparing base (422adfd) to head (de1301d).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5518   +/-   ##
=======================================
  Coverage   86.60%   86.60%           
=======================================
  Files         590      590           
  Lines       42705    42705           
  Branches     7096     7096           
=======================================
  Hits        36985    36985           
  Misses       5720     5720           
Flag Coverage Δ
unittests 86.60% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

declare module "ace-code/src/mode/swift_highlight_rules" {
export const SwiftHighlightRules: new () => import(".").Ace.HighlightRules;
export const HighlightRules: new () => import(".").Ace.HighlightRules;
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that I added SwiftHighlightRules in #5516, which one should we export here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, the script adds both types of exports. It collects all exports and generates declarations based on them. I don’t see this as a problem, especially considering that it accurately reflects the current codebase. However, if desired, I could add an exclusion list for exports we prefer not to include.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see, I've just merged the change after this PR is created and couldn't see it reflected. Will it be included in the next time we run this script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I merged new changes and run script again -> now there are two exports

Copy link

One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR.

@oykuyilmaz
Copy link
Contributor

Hey mkslanc,
Regarding the discussion point on "Integration Timing":
We talked about the possibility of running this script in a pre-commit hook. This way, a Github action can later compare the newly created ace-modes.d.ts file with the one in the main branch. If different, this action could comment that the requester should create the new ace-modes.d.ts by running this script and commit it in the PR. Does this make sense? Are you able to create such Github actions in this repo?

@mkslanc
Copy link
Contributor Author

mkslanc commented Mar 26, 2024

Hey mkslanc, Regarding the discussion point on "Integration Timing": We talked about the possibility of running this script in a pre-commit hook. This way, a Github action can later compare the newly created ace-modes.d.ts file with the one in the main branch. If different, this action could comment that the requester should create the new ace-modes.d.ts by running this script and commit it in the PR. Does this make sense? Are you able to create such Github actions in this repo?

Hey! I think it makes sense, but I am not sure, if I have permission to create/modify github actions in this repo.

@InspiredGuy
Copy link
Contributor

@mkslanc github action is just a yaml file in .github/workflows, you don't need extra access level for it I think. See #5506 for example of adding an action. For pre-commit hook I think we might use husky as a devDependency.

I think it would be best if the generation could be run both manually an in pre-commit hook, with our contribution guide mentioning this script as well.

@nightwing nightwing force-pushed the ace-modes-dts-generator branch 8 times, most recently from 659a877 to d3fde97 Compare March 31, 2024 22:06
@ajaxorg ajaxorg deleted a comment from github-actions bot Mar 31, 2024
@ajaxorg ajaxorg deleted a comment from github-actions bot Mar 31, 2024
@ajaxorg ajaxorg deleted a comment from github-actions bot Mar 31, 2024
@ajaxorg ajaxorg deleted a comment from github-actions bot Mar 31, 2024
@ajaxorg ajaxorg deleted a comment from github-actions bot Mar 31, 2024
@ajaxorg ajaxorg deleted a comment from github-actions bot Mar 31, 2024
@ajaxorg ajaxorg deleted a comment from github-actions bot Mar 31, 2024
@ajaxorg ajaxorg deleted a comment from github-actions bot Mar 31, 2024
@nightwing nightwing force-pushed the ace-modes-dts-generator branch 3 times, most recently from 51a88d7 to ff4b0de Compare March 31, 2024 22:49
@nightwing nightwing force-pushed the ace-modes-dts-generator branch 8 times, most recently from dd44e9a to 3923d41 Compare April 1, 2024 20:17
Copy link

github-actions bot commented Apr 1, 2024

One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR.

@ajaxorg ajaxorg deleted a comment from github-actions bot Apr 1, 2024
@ajaxorg ajaxorg deleted a comment from github-actions bot Apr 1, 2024
@ajaxorg ajaxorg deleted a comment from github-actions bot Apr 1, 2024
@ajaxorg ajaxorg deleted a comment from github-actions bot Apr 1, 2024
@ajaxorg ajaxorg deleted a comment from github-actions bot Apr 1, 2024
@ajaxorg ajaxorg deleted a comment from github-actions bot Apr 1, 2024
@ajaxorg ajaxorg deleted a comment from github-actions bot Apr 1, 2024
@ajaxorg ajaxorg deleted a comment from github-actions bot Apr 1, 2024
@ajaxorg ajaxorg deleted a comment from github-actions bot Apr 1, 2024
@ajaxorg ajaxorg deleted a comment from github-actions bot Apr 1, 2024
@ajaxorg ajaxorg deleted a comment from github-actions bot Apr 1, 2024
@nightwing
Copy link
Member

I have added a ci script to check that types are updated.

I think we should leave husky as a follow-up pull request, because the situation with updating types is same us the situation with npm run fix before, and it may be better to use ci bot to add the changes to prs instead of using husky script that may not work on some systems, or when creating pull request from github edit.

@nightwing nightwing merged commit e59ae67 into ajaxorg:master Apr 5, 2024
4 checks passed
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.

4 participants