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

v7 prep - includes redesigning parts of core, and unifying different types #956

Merged
merged 35 commits into from
Jun 1, 2021

Conversation

devoidfury
Copy link
Contributor

@devoidfury devoidfury commented May 24, 2021

The intent of this PR is to do a few things -

  • Refactor parts of core to better suit the newer declarative API
  • Simplify underlying code where possible
  • Unify and remove duplicate implementations of the same underlying type
  • Make the API a bit more consistent
  • Inline some or all of the XSD schemas for the types we're using as comments, as discussed in Add validator (Nonconformance to Office Open XML schema) #947
  • Clean up output values to be conformant to the spec, throw errors instead of silently failing

It's not ready to merge quite yet - I'm still working on things here; so far I've removed more code than I've added, even after adding a lot of comments and a few features. Opening this PR so you can review what's been done so far and start the discussion around these changes.

There may be a few items worth cherry-picking onto v6 - for example, commit 528be93, which fixes issue #507.

@codecov-commenter
Copy link

codecov-commenter commented May 24, 2021

Codecov Report

Merging #956 (034cd18) into master (37d099b) will increase coverage by 0.56%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #956      +/-   ##
==========================================
+ Coverage   98.73%   99.29%   +0.56%     
==========================================
  Files         339      311      -28     
  Lines        3859     3541     -318     
  Branches      354      377      +23     
==========================================
- Hits         3810     3516     -294     
+ Misses         48       24      -24     
  Partials        1        1              
Impacted Files Coverage Δ
src/export/packer/next-compiler.ts 90.00% <ø> (-0.17%) ⬇️
src/file/document/document.ts 100.00% <ø> (ø)
src/file/drawing/anchor/anchor.ts 100.00% <ø> (ø)
src/file/drawing/drawing.ts 100.00% <ø> (+16.66%) ⬆️
src/file/drawing/extent/extent.ts 100.00% <ø> (+22.22%) ⬆️
...rawing/inline/graphic/graphic-data/graphic-data.ts 100.00% <ø> (+18.18%) ⬆️
...ile/drawing/inline/graphic/graphic-data/pic/pic.ts 100.00% <ø> (+13.33%) ⬆️
...-data/pic/shape-properties/form/extents/extents.ts 100.00% <ø> (+22.22%) ⬆️
...hic/graphic-data/pic/shape-properties/form/form.ts 100.00% <ø> (+12.50%) ⬆️
...phic-data/pic/shape-properties/shape-properties.ts 100.00% <ø> (+15.38%) ⬆️
... and 98 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37d099b...034cd18. Read the comment docs.

@dolanmiu
Copy link
Owner

Wow, very well done,

Could you add a list of breaking changes so I can add it to the release notes, like so:

https://github.com/dolanmiu/docx/releases

@devoidfury
Copy link
Contributor Author

devoidfury commented May 24, 2021

Yes, I'll make you a list when it's ready. I still need to go over:

  • export/
  • file/document
  • file/drawing
  • file/numbering
  • file/settings
  • file/styles

and a couple other areas first.

@devoidfury
Copy link
Contributor Author

devoidfury commented May 26, 2021

I'm wrapping it up, that's good enough for now. I did not get deep into file/drawing -- that should probably be done at some point, but for now we can move ahead without reworking it.

Here's the breaking changes and notes:

New features:

  • In several places, as allowed by the spec, Universal Measure units can now be used.
    Universal Measure units are strings in the format /-?[0-9]+(.[0-9]+)?(mm|cm|in|pt|pc|pi)/, for example: "14pt", "20mm", "2in", "2cm".
    These include page margins, page size, paragraph indents, and font sizes. These should be experimented with, some of these may not work and need testing -- even if it's technically valid according to the spec.

  • Added a bunch of formats to NumberFormat.

  • Much more compliant docx files, which should act more consistently in different editors.

  • Reduced bundle size slightly.

  • Boolean options like bold, etc can now be explicitly toggled off -- when set to false it adds the tag to disable it, rather than just omit the tag to enable it, which allows more flexibility.

Breaking changes:

  • Renamed enum PageNumberFormat -> NumberFormat.

  • Section vertical align enum changed: SectionVerticalAlignValue -> VerticalAlign

  • Runtime checks were added for some values. These can now throw errors for invalid arguments in some cases.

  • color values must now be in hex (eg "00CCFF") for Underline, Shading, run color, background color, border color

  • Paragraph border argument signature changed.
    old:

  new Paragraph({
      border: {
          top: {
              value: "single",
              size: 6,
              color: "red",
          },
      }
  })

new:

  new Paragraph({
      border: {
          top: {
              style: BorderStyle.SINGLE,
              size: 6,
              color: "FF0000",
          },
      }
  })
  • For TableOfContents, updateFields option in Document must be explicitly set:
new Document({
  features: {
      updateFields: true,
  },
  // ...
})
  • ITableShadingAttributesProperties changed to IShadingAttributesProperties

  • Table and TableCell shading option signature changed to be consistent with shading everywhere else.

old:

  shading: {
      val: ShadingType.REVERSE_DIAGONAL_STRIPE,
      color: "00FFFF",
      fill: "FF0000",
  },

new:

  shading: {
      type: ShadingType.REVERSE_DIAGONAL_STRIPE,
      color: "00FFFF",
      fill: "FF0000",
  },
  • A great many internal classes were shifted around or removed. Internal methods were removed in several places. These changes should not affect you if you stuck to the public API (what's in the demos and docs)

@dolanmiu
Copy link
Owner

dolanmiu commented May 29, 2021

@devoidfury Been reviewing it recently

Will let you know!

docs/usage/text.md Outdated Show resolved Hide resolved
src/convenience-functions.ts Show resolved Hide resolved
src/file/core-properties/properties.ts Show resolved Hide resolved
src/file/paragraph/properties.ts Show resolved Hide resolved
src/file/styles/style/paragraph-style.ts Show resolved Hide resolved

for (const child of options.children) {
this.root.push(child);
}

if (options.width) {
Copy link
Owner

Choose a reason for hiding this comment

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

Are these properties added back anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

src/file/table/table-cell/table-cell-properties.ts

src/file/xml-components/simple-elements.ts Show resolved Hide resolved
@dolanmiu
Copy link
Owner

@devoidfury Left some comments

Feel free to reply and address, will approve after!

@@ -132,18 +134,6 @@ export class File {
this.footnotesWrapper.View.createFootNote(parseFloat(key), options.footnotes[key].children);
}
}

if (options.features) {
if (options.features.trackRevisions) {
Copy link
Owner

@dolanmiu dolanmiu May 29, 2021

Choose a reason for hiding this comment

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

@dolanmiu
Copy link
Owner

@devoidfury Does demo 60 still work?

@devoidfury
Copy link
Contributor Author

@dolanmiu it appears to - I didn't remove the "trackRevisions" feature

@devoidfury
Copy link
Contributor Author

src/file/settings/settings.ts

Rather than calling this.settings.addTrackRevisions(), it's passed straight as an option into the Settings object which handles it in the constructor.

@dolanmiu
Copy link
Owner

@devoidfury Thank you for clarifying

@dolanmiu dolanmiu merged commit de145e0 into dolanmiu:master Jun 1, 2021
@dolanmiu
Copy link
Owner

dolanmiu commented Jun 1, 2021

@devoidfury Merged in!

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.

None yet

3 participants