OfficeDev / Office-Addin-Scripts

A set of scripts and packages that are consumed in Office add-ins projects.
MIT License
152 stars 93 forks source link

[patch] custom-function-metadata #836

Closed wandyezj closed 3 months ago

wandyezj commented 4 months ago

Change Description

Refactor custom-function-metadata to separate parseTree from generateCustomFunctionMetadata. This allows parseTree to be used in the browser without having to polyfill fs.

break generate.ts into two files: parseTree.ts and generateCustomFunctionMetadata.ts.

update main.ts exports to specify exact types that are exported.

  1. Do these changes impact command syntax of any of the packages? (e.g., add/remove command, add/remove a command parameter, or update required parameters) No

  2. Do these changes impact documentation? (e.g., a tutorial on https://learn.microsoft.com/office/dev/add-ins/overview/office-add-ins) No

Validation/testing performed:

Ran unit tests.
akrantz commented 4 months ago

Are these changes driven from ScriptLab code changes? Is there a PR there for the usage of this.

There's no description in this PR about the changes -- what they are, or why the changes are being made.

wandyezj commented 4 months ago

Are these changes driven from ScriptLab code changes? Is there a PR there for the usage of this.

There's no description in this PR about the changes -- what they are, or why the changes are being made.

See PR description:

Refactor custom-function-metadata to separate parseTree from generateCustomFunctionMetadata.
This allows parseTree to be used in the browser without having to polyfill fs.

break generate.ts into two files: parseTree.ts and generateCustomFunctionMetadata.ts.
wandyezj commented 4 months ago

I'd prefer to keep gnerate.ts more as it was. Would like to understand if there is some plication about parseTree functionality and integration with ScriptLab which is being made here.

It is challenging to make this work.

  1. It's clear that other packages are importing generate directly instead of relying on the package index - as evidenced by test suite failures when not everything is exported from generate.ts. Yes, referencing files directly instead of using the index is generally considered a bad practice. However, it's convenient to avoid breaking existing usage.
  2. parseTree needs to be in its own file separate from files that are importing fs.

This devolves to the existing refactor of separate files for parseTree and generateCustomFunctionMetadata and then exporting these as done previously from generate.ts to avoid breakage.

It's also worth looking at root causes about why we are importing from generate.ts directly instead of the index file. The root cause here is it's really confusing what is exported from this package since the exports are not clear from the index. I fix that here as well to encourage proper importing.

millerds commented 3 months ago

Appears to have been abandoned.