Skip to content

Conversation

@lforst
Copy link

@lforst lforst commented May 23, 2023

Will auto detect build artifacts based on the information bundlers provide in the build end hooks - making the assets option optional.

Risky change incoming.

@lforst lforst requested review from HazAT and Lms24 May 23, 2023 09:00
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

I agree it's a risky change but the implementation makes sense to me. Nevertheless, I think we should cover this with at least one integration or e2e test, wdyt?

debugIdChunkFilePath.endsWith(".cjs")
);

if (Array.isArray(assets) && assets.length === 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Is assets always normalized to an array? otherwise, what about assets: ""?

Copy link
Author

Choose a reason for hiding this comment

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

My thought process here was to just skip uploading entirely when users provide an empty array (as a means of disabling I guess). An empty string is more or less user error which we don't care to catch here. I think this will be implicitly caught by the glob pattern not finding anything and the message below then saying "check your assets option".

Copy link
Member

Choose a reason for hiding this comment

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

Oh good point. At the time of writing I thought for some reason an empty array and an empty string the same semantic implications here but you're right, an empty string looks more like an error than an empty array.

debugIdChunkFilePath.endsWith(".cjs")
);
const debugIdChunkFilePaths = (
await glob(assets ?? buildArtifactPaths, {
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a log that we're using the auto-determined paths if we fall back to buildArtifactPaths?

Copy link
Author

Choose a reason for hiding this comment

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

debug or info level?

@lforst lforst merged commit 707c8c9 into main May 24, 2023
@lforst lforst deleted the lforst-auto-detect-build-artifacts branch May 24, 2023 09:06
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.

3 participants