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

Add an option to preserve assets in the output directory #1163

Merged
merged 11 commits into from
May 17, 2023
Merged

Conversation

mrm007
Copy link
Contributor

@mrm007 mrm007 commented May 5, 2023

This is needed for packages which ship Vocab translations, but can be useful for packages which publish CSS, images etc.

How it works

There's a new config key named assets:

// package.json
  "skuba": {
    "assets": [
      "**/*.vocab/*translations.json"
    ],
    // ...
  }

This will instruct skuba to copy the files matching the list of globs to the output directory/ies, preserving the directory structure from src:

  • for skuba build-package it will copy them to lib-commonjs and lib-es2015
  • for skuba build it will copy them to lib

Context behind this change

This came about when we started the work of compiling Metropolis packages. As part of that initiative, we've made some changes to Vocab. As of version 1.2.0 of the Webpack plugin, it can now process compiled code, meaning that it will look for translations.json files next to the generated compiled .vocab/index.ts file.

That means that consumers a package built with skuba (and SEEK has a few) will start seeing failing builds because the package does not ship translations.json files. This PR fixes that, but it is not the solution. Vocab was not meant to work with compiled code, but this will work for now. We'll need to revisit how Vocab works in the future.

Some notes about this PR

There were a few threads on Slack with @askoufis, @72636c and @georgespyropoulos about the best way to do this. We decided to go with the simplest solution for now, which is to add a new assets key to package.json#skuba which will tell skuba which assets to copy to the output directory/ies. This is similar to how package.json#files works.

Currently, these's no documentation about this new feature, but we'll add it soon. What is the best place to document it? The skuba config is not documented anywhere at the moment. I thought about adding it in skuba init and/or skuba configure. @72636c suggested it might be time for a proper skuba.config.ts, especially if we want to support some defaults for Vocab.
For now, you can see the tests for how it works.

When using skuba build with "build": "esbuild" in the skuba config, esbuild will not compile .vocab/index.ts translation files generated by Vocab. That was a bit weird, so it must be something telling TypeScript/esbuild to ignore those files. I didn't dig deeper and it's unrelated to this PR, but something that would need to be addressed before the next major release when esbuild becomes the default. Edit: fixed in #1164

@mrm007 mrm007 requested review from a team as code owners May 5, 2023 03:28
@changeset-bot
Copy link

changeset-bot bot commented May 5, 2023

🦋 Changeset detected

Latest commit: e9edc38

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
skuba Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

@askoufis askoufis left a comment

Choose a reason for hiding this comment

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

LGTM 👍

src/cli/build/assets.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@samchungy samchungy left a comment

Choose a reason for hiding this comment

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

Lgtm otherwise nice one 🙏 @72636c you're probably more across this code

Copy link
Member

@72636c 72636c left a comment

Choose a reason for hiding this comment

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

This is great. I'd like to raise the skuba.config.ts proposal for review but that can be a separate PR 👍

src/cli/build/index.ts Outdated Show resolved Hide resolved
src/cli/build/assets.ts Outdated Show resolved Hide resolved
return;
}

const resolvedSrcDir = path.resolve(path.dirname(manifest.path), 'src');
Copy link
Member

Choose a reason for hiding this comment

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

This is interesting as we haven't taken a strong opinion on src within the tooling itself, it's just the templated default. Would it make sense for assets to be specified relative to the parent directory (alongside package.json)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made the change here 3a6232e (#1163). I think a better approach would be to use the directory from skuba.entryPoint. What do you think?

@samchungy samchungy mentioned this pull request May 9, 2023
@72636c 72636c mentioned this pull request May 9, 2023
@mrm007 mrm007 enabled auto-merge (squash) May 17, 2023 06:40
@mrm007 mrm007 disabled auto-merge May 17, 2023 06:40
@mrm007 mrm007 merged commit d1b6ba9 into master May 17, 2023
19 checks passed
@mrm007 mrm007 deleted the preserve-assets branch May 17, 2023 06:51
@seek-oss-ci seek-oss-ci mentioned this pull request May 17, 2023
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

4 participants