feat: Modify image file names

[andrew-polk]

Breaking change:
Previously, image files names were a hash of all or part of the image url.
To provide more stability and future-proofing, the default format is now {page-slug}.{notion-block-id}.
Users can opt in to the old format with --image-file-name-format legacy.

---

Feature:
If desired instead, users can specify --image-file-name-format content-hash to use a hash of the image content as the file name.

---

Potential breaking change for plugins:
The exported type IDocuNotionContext changed from

export type IDocuNotionContext = { 
  layoutStrategy: LayoutStrategy; 
  options: DocuNotionOptions; 
  getBlockChildren: IGetBlockChildrenFn; 
  notionToMarkdown: NotionToMarkdown;
  directoryContainingMarkdown: string; 
  relativeFilePathToFolderContainingPage: string; 
  convertNotionLinkToLocalDocusaurusLink: (url: string) => string | undefined; 
  pages: NotionPage[]; 
  counts: ICounts; 
  imports: string[]; 
}; 

to

export type IDocuNotionContext = { 
  layoutStrategy: LayoutStrategy; 
  options: DocuNotionOptions; 
  getBlockChildren: IGetBlockChildrenFn; 
  notionToMarkdown: NotionToMarkdown; 
  pageInfo: IDocuNotionContextPageInfo; 
  convertNotionLinkToLocalDocusaurusLink: (url: string) => string | undefined; 
  pages: NotionPage[]; 
  counts: ICounts; 
  imports: string[]; 
}; 

where IDocuNotionContextPageInfo is

export type IDocuNotionContextPageInfo = { 
  directoryContainingMarkdown: string; 
  relativeFilePathToFolderContainingPage: string; 
  slug: string; 
}; 

---

This change is 

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[JohnThomson]

Reviewed 13 of 13 files at r1.
Reviewable status: 12 of 13 files reviewed, 3 unresolved discussions (waiting on @andrew-polk)

---

src/MakeImagePersistencePlan.ts line 42 at r1 (raw file):

    const hash = hashOfString(thingToHash);
    imageSet.outputFileName = `${hash}.${imageFileExtension}`;
  } else if (options.imageFileNameFormat === "content-hash") {

This is not present for backwards compatibility and the result won't be stable against any change to the image. Maybe a comment on why we think it might be useful?

---

src/MakeImagePersistencePlan.ts line 60 at r1 (raw file):

      ? `${imageSet.pageInfo.slug.replace(/^\//, "")}.`
      : "";
    imageSet.outputFileName = `${pageSlugPart}${imageBlockId}.${imageFileExtension}`;

The documentation says the original file name will be used "if it can be determined" but I don't see any hint of that here.
My recollection is that we planned not to allow pages without slugs any more. If that's right, should we report an error here? If we do allow missing page slugs, can we be sure imageBlockId will be unique across all pages that don't have slugs? Or is there something else that will stop our images overwriting each other? May be worth a comment. (Later: I see that requiring slugs is an independent command line option. I'll let you decide if any additional comment is useful.)
Possibly also worth commenting how we can be sure imageBlockId is stable (or why it doesn't matter).

---

src/MakeImagePersistencePlan.ts line 66 at r1 (raw file):

    imageOutputRootPath?.length > 0
      ? imageOutputRootPath
      : imageSet.pageInfo!.directoryContainingMarkdown,

Was there a bug before? I don't see a pathToParentDocument in the old IDocuNotionContext.

[andrew-polk]

Reviewable status: 10 of 13 files reviewed, 3 unresolved discussions (waiting on @JohnThomson)

---

src/MakeImagePersistencePlan.ts line 42 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

This is not present for backwards compatibility and the result won't be stable against any change to the image. Maybe a comment on why we think it might be useful?

Done.

---

src/MakeImagePersistencePlan.ts line 60 at r1 (raw file):

The documentation says the original file name will be used "if it can be determined" but I don't see any hint of that here.

Oops. I forgot to remove that from the doc when we decided to omit it. Good catch.

I think I've addressed the rest of what you wrote by adding a comment.

---

src/MakeImagePersistencePlan.ts line 66 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Was there a bug before? I don't see a pathToParentDocument in the old IDocuNotionContext.

Previously, processImageBlock had

  imageSet.pathToParentDocument = pathToParentDocument;

where pathToParentDocument was a local parameter sourced eventually by IDocuNotionContext's fullPathToDirectoryContainingMarkdown.

Now it is

  imageSet.pageInfo = context.pageInfo;

So, no, it wasn't a bug. I'm just passing the whole context and then the page context instead of the particular properties.
And those page properties were named differently in ImageSet and IDocuNotionContext.

[JohnThomson]

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @andrew-polk)

---

src/MakeImagePersistencePlan.ts line 44 at r3 (raw file):

  } else if (options.imageFileNameFormat === "content-hash") {
    // This was requested by a user: https://github.com/sillsdev/docu-notion/issues/76.
    // We considering including this as well, but we want to maintain as much stability in the file name

Something went wrong with that sentence. "We considered"? "We are considering"? (But here it is)
I suppose another possible benefit would be if you WANT to force the localizers to redo the localization of any image that changes.
Also, possibly because of that, I'm not quite sure what 'this' is.

• We considered adding the content-hash option to the command line (but we have)
• We considered making content-hash the default?
• We considered including content-hash in the default result (along with the slug instead of the objectId)?

[andrew-polk]

Reviewable status: 12 of 13 files reviewed, 1 unresolved discussion (waiting on @JohnThomson)

---

src/MakeImagePersistencePlan.ts line 44 at r3 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Something went wrong with that sentence. "We considered"? "We are considering"? (But here it is)
I suppose another possible benefit would be if you WANT to force the localizers to redo the localization of any image that changes.
Also, possibly because of that, I'm not quite sure what 'this' is.

• We considered adding the content-hash option to the command line (but we have)
• We considered making content-hash the default?
• We considered including content-hash in the default result (along with the slug instead of the objectId)?

Yeah, the we considered comment is out of place. I was thinking in terms of our team was thinking we might make use of the content hash in our own file names. But this library is outside the context of our use of it.
I'll try to clarify.

[JohnThomson]

but Reviewable won't let me do an actual approving review

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @andrew-polk)

[github-actions]

🎉 This PR is included in version 0.14.0-alpha.15 🎉

The release is available on:

• npm package (@alpha dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[github-actions]

🎉 This PR is included in version 0.16.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀