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

[heft] Introduce initial implementation of 'copyFiles' action #2333

Merged
merged 35 commits into from
Nov 10, 2020

Conversation

D4N14L
Copy link
Member

@D4N14L D4N14L commented Nov 2, 2020

Adds a new default Heft action named copyFiles. This action can run in all build events except clean, as there should be no need to copy any files on a clean.

An example implementation of the schema to add the event is here:

{
      "actionKind": "copyFiles",
      "heftEvent": "pre-compile",
      "actionId": "defaultCopyFiles",
      "copyOperations": [
          {
             "sourceFolder": "src" // Optional, defaults to project root
             "fileExtensions": [ ".json", ".txt" ], // Optional
             "includeGlobs": [ "**/*.md" ], // Optional
             "excludeGlobs": [ "**/__test__/**/*.md" ], // Optional
             "destinationFolders": [ "dist" ], // Required
             "hardlink": false, // Optional, defaults to false
             "flatten": false // Optional, defaults to false
          }
      ]
    }

Some use cases for this plugin could include:

  • copying checked-in assets that may not be required for Typescript build, or may not exist in the src directory, as CopyStaticAssetsPlugin is used for
  • copying build assets after a build completes to some directory

Reasons for glob (which has perf implications) instead of a more targeted folder/file copy design:

  • flexibility provided by the plugin can be used in various scenarios, from copying entire directories or subsets of entire directories to copying specific file extensions or specific file names
  • targeted folder/file copy schema would be more verbose, possibly needing regular heft.json updates in order to keep up-to-date with which files need copying

@D4N14L D4N14L changed the title [heft] Introduce initial implementation of 'copyGlobs' event [heft] Introduce initial implementation of 'copyGlobs' action Nov 2, 2020
@D4N14L D4N14L changed the title [heft] Introduce initial implementation of 'copyGlobs' action [heft] Introduce initial implementation of 'copyFiles' action Nov 4, 2020
apps/heft/src/plugins/CopyFilesPlugin.ts Outdated Show resolved Hide resolved
apps/heft/src/plugins/CopyFilesPlugin.ts Outdated Show resolved Hide resolved
apps/heft/src/plugins/DeleteGlobsPlugin.ts Outdated Show resolved Hide resolved
apps/heft/src/schemas/heft.schema.json Show resolved Hide resolved
apps/heft/src/plugins/CopyFilesPlugin.ts Outdated Show resolved Hide resolved
apps/heft/src/plugins/CopyFilesPlugin.ts Outdated Show resolved Hide resolved
Comment on lines 186 to 191
const pattern: string = `**/*+(${escapedExtensions.join('|')})`;
sourceFileRelativePaths = await this._expandGlobPatternAsync(
resolvedSourceFolderPath,
pattern,
copyConfiguration.excludeGlobs
);
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this would be faster if we crawled the filesystem explicitly and looked for files matching one of the extensions instead of constructing and evaluating a glob.

Copy link
Member Author

Choose a reason for hiding this comment

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

Switched to fast-glob. We talked offline, but under the covers this uses micromatch which is what I was planning on using anyway, alongside a manual crawl of the directory. We're also able to do a single call with multiple different glob patterns in one go, avoiding enumerating the filesystem multiple times when running patterns in the same folder. Ideally this should have the same perf as a custom solution, save for the additional calls we would have to make when targeting other sourceFolders

Another thought here would be that we could combine all globs across all copy configurations, while appending the individual sourceFolder path to the glob, though I'm unsure if this would cause a perf hit, ie:

{
  "sourceFolder": "src/subfolder",
  "includeGlobs": [ "**/*.json", "**/*.md" ] // -> "includeGlobs": [ "src/subfolder/**/*.json", "src/subfolder/**/*.md" ]
  ...
}

apps/heft/src/plugins/CopyFilesPlugin.ts Outdated Show resolved Hide resolved
apps/heft/src/plugins/CopyFilesPlugin.ts Show resolved Hide resolved
@D4N14L D4N14L requested a review from patmill as a code owner November 10, 2020 03:24
@octogonz
Copy link
Collaborator

I've approved this PR. I don't want to block it. But I'm still not 100% sure about the perf implications of our design.

Ideally the config file should encourage developers NOT to write globs that do an O(n) scan of every subdirectory. Optimizing our globs is also good 👍 👍 , but NOT using globs in the first place will probably always be faster. Our design shouldn't prevent globs, just make it clear when they are inefficient.

The idea I originally advocated seems to end up like this, which is complicated and awkward:

  • filesToInclude
  • foldersToInclude
  • foldersToExclude
  • globsToInclude
  • globsToExclude

Maybe a simpler idea would be:

  • globsToInclude
  • globsToExclude
  • allowSlowGlobs

In other words, what if our engine detected inefficient globs (e.g. **), such that the person had to explicitly set allowSlowGlobs=true to say "yes, I realize my approach is going to be inefficient."

The fast-glob engine distinguishes static-vs-dynamic patterns, which maybe is the fast-vs-slow concept.

Just something to think about... again, I don't want to hold up this PR.

@iclanton iclanton merged commit 462a377 into microsoft:master Nov 10, 2020
@iclanton iclanton deleted the danade/copy-plugin branch November 10, 2020 23:00
@octogonz
Copy link
Collaborator

The JSON schema changes from this PR have been published.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants