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

A number of changes to make the TypeScript target fully usable. #4408

Closed
wants to merge 0 commits into from

Conversation

mike-lischke
Copy link
Member

@mike-lischke mike-lischke commented Sep 5, 2023

This is a pretty large patch involving these changes:

  • Added many missing copyright headers.
  • Changed all type definitions to use a default export, in cases where they were missing.
  • Cleaned up existing type definitions (including tab -> whitespace conversion and formatting).
  • Added a number of new type definitions.
  • Changed occurrences of undefined to null for consistency.
  • Imports now consistently use the .js extension, as required for full ESM support.
  • Added TokenFactory and Vocabulary implementations.
  • Fixed export sections in the main index files.
  • Moved babel settings to package.json, which avoids the need for a separate config file.
  • Made final bundle even smaller by excluding files that do not belong to the release (e.g. original source files, specs, config files).
  • Updated TypeScript stg file (mostly whitespaces, but also support for vocabulary and updated imports, override where necessary and such things).
  • Fixed wrong type parameter for Recognizer, which is used for the used interpreter, so Token or number are completely wrong.
  • Switched from ts-node to tsx, because ts-node failed to run, while tsx works out of the box (there's a large number of complains for ts-node being too picky).
  • Added all the XPath stuff.
  • Removed RuleNode, Tree and SyntaxTree classes, which are actually interfaces only used for ParseTree, so they have been combined into that ParseTree, except for RuleNode which contained only a single method, which is implemented in RuleContext, hence it was moved to RuleContext.d.ts.
  • Changed the name of the auto generated list() methods in TypeScript.stg ot use a double underscore instead of a single one because it's much more likely to get a conflict with the single underscore. The double underscore is not 100% safe either, but I expect it to be much less common for rule names.

@mike-lischke
Copy link
Member Author

@ericvergnaud This is the one to review, with some more details what I changed.

@mike-lischke
Copy link
Member Author

Side note: the current JS/TS target doesn't work with vite (regardless of this patch), so this requires another patch once I figured out what's going on. But I keep that separate to make this already large patch even larger.

@ericvergnaud
Copy link
Contributor

Hi Mike, thanks for this.

Would it be possible for you to breakdown this PR into smaller, focused PRs ?

  • default exports
  • vocabulary
  • token factory
    ...
    such that we can keep track of which changes have which impact, and also discuss your proposals on a more granular basis ?

@mike-lischke
Copy link
Member Author

Would it be possible for you to breakdown this PR into smaller, focused PRs ?

Let me think about it. The main problem is that each individual PR must work on its own and that's not easy to accomplish, let alone fighting with intermediate other PRs like I just had and which costs me a lot of some time to fix the conflicts.

@ericvergnaud
Copy link
Contributor

There's no need to submit all these PRs at once
You could start with vocabulary, which requires a tool release
Once merged we look into the next one...

@mike-lischke
Copy link
Member Author

Regardless how the individual PRs are submitted, they all have to work on their own. Some PRs will still be large (e.g. the cleanup, which touches many files) so there will be a large PR anyway, regardless of what else can be separated.

Tbh. I have a hard time to see how to split this patch, because cleanup and addition of stuff to type definitions go hand in hand. I rather fear to introduce errors...

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.

2 participants