microsoft / monosize

Bundle size tooling for monorepos
MIT License
25 stars 9 forks source link

Feature(monosize): make `packageName` configurale within `compare-reports` command #62

Closed Hotell closed 3 months ago

Hotell commented 4 months ago

ATM packageName for local report is being inferred from package folder basename https://github.com/microsoft/monosize/blob/main/packages/monosize/src/utils/collectLocalReport.mts#L23

While this might work for simple use-cases once it should be configurable by the user.

Example

when directory structure is (see within PR)

|-react-alert/
  |- library/
    |- /bundle-size 
  |- stories/
  |- e2e/

Current:

image

Expected:

image

Proposed Implementations:

  1. change the behaviours to obtain the name from package.json#name and fallback to folder basename if not present

This would be a BREAKING CHANGE

  1. enable set this behaviour via configuration
monosize compare-reports --packageName='folder'|'package.json'
export type MonoSizeConfig = {
    repository: string;
    storage: StorageAdapter;
    bundler: BundlerAdapter;
+   commands: {
+     'compare-report': {
+        packageName: 'directory' | 'packageJSON' | ( packageRoot:string ) => string
+      }
  }
};
layershifter commented 4 months ago

@Hotell oopsie, good catch 👍


packageName is defined there and is used also for upload-report:

https://github.com/microsoft/monosize/blob/2db473d8effc2fe2288d9c5f33b0db721e9cd359/packages/monosize/src/utils/collectLocalReport.mts#L23-L24

As it's used in multiple places IMO it should be hosted in a config. As monosize is still in 0.x, IMO it's fine to break it for good 🐱

I propose:

export type MonoSizeConfig = {
    repository: string;
    storage: StorageAdapter;
    bundler: BundlerAdapter;
+   packageName?: async (packageRoot: string) => Promise<string>;
};

WDYT?

Hotell commented 4 months ago

For sake of simplicity this is good enough IMO from configuration POV, not sure about the defaults (inferring name from package.json):

but I'm ok to go with this and provide appropriate error messages to user.

WDYT?

layershifter commented 4 months ago

For sake of simplicity this is good enough IMO from configuration POV, not sure about the defaults (inferring name from package.json):

  • If user library(package) doesn't contain package.json, monosize would provide invalid results/throw error by default.
  • now some libraries might use only project.json (nx ecosystem) for non publishable packages (another case of invalid output )

That's a good call out. Then let's keep directory name as a default? So there won't be breaks at all.

If we want to handle these scenarios better, we also need to make findPackageRoot() configurable as currently it accounts only for package.json:

https://github.com/microsoft/monosize/blob/2db473d8effc2fe2288d9c5f33b0db721e9cd359/packages/monosize/src/utils/collectLocalReport.mts#L12

Hotell commented 4 months ago

so to recap the solution we wanna go forward with:

Additional thoughts:

so if we update package root retrieval to account for project.json or package.json why not leverage it to get the name property from there already ==> this would imply default name resolution like (package.json#name ?? project.json#name ?? directory-name)

layershifter commented 4 months ago

Additional thoughts:

so if we update package root retrieval to account for project.json or package.json why not leverage it to get the name property from there already ==> this would imply default name resolution like (package.json#name ?? project.json#name ?? directory-name)

What about this plan then? ⬇️

Hotell commented 4 months ago

sounds good 🤝 !

I'll implement this next week. ty !