BuildSize icon indicating copy to clipboard operation
BuildSize copied to clipboard

Files with the same names get overriden

Open tusbar opened this issue 7 years ago • 9 comments

If i have something like this

extension
├── _locales
│   ├── en
│   │   └── messages.json
│   └── fr
│       └── messages.json
├── manifest.json

I only get one messages.json in the BuildSize output:

File name Previous Size New Size Change
manifest.json 745 bytes 828 bytes 83 bytes (11%)
messages.json 1.12 KB 1.12 KB 0 bytes (0%)

We should probably do something with partial paths for files with equal names, something like this would be nice:

File name Previous Size New Size Change
manifest.json 745 bytes 828 bytes 83 bytes (11%)
en/messages.json 1.12 KB 1.12 KB 0 bytes (0%)
fr/messages.json 1.12 KB 1.12 KB 0 bytes (0%)

tusbar avatar Apr 10 '18 17:04 tusbar

Good idea. I don't have duplicate file names in any of my projects so it hasn't been an issue for me.

Something to consider: What do we do if there's just one messages.json? Do we display it as messages.json or en/messages.json? If we just display the file name, then what do we do if a new messages.json is added?

I guess we should store the full path, and just trim it for display, based on whether there's other files with the same file name. Just need to be careful that we don't break existing projects.

Daniel15 avatar Apr 10 '18 17:04 Daniel15

For readability of the table we should keep the names short, so if there is only one message.json we should keep it as is.

Yeah, we should store the full paths and display shorter ones. And we could even do something like this:

| File name |
| --------- |
| <div title="_locales/messages.json">messages.json</div> |
File name
messages.json

Which would display the full path on hover.


We need to make sure that the file name we display is not too long, for example if we have duplicated file names in deep folder structures, for example foo/bar/one/two/messages.json and messages.json, we can display something like foo/.../messages.json and messages.json.

Maybe there’s a lib that does that.

If a new messages.json one is added, if the key is the full path and the filename shown is only for the comment, the name will change from other PRs in the table but won’t break anything.


At least having duplicated entries in the table with the title attribute would be a nice first step.

tusbar avatar Apr 10 '18 17:04 tusbar

We should also sort the table by path/name when this gets added. :)

Today it looks like new found resources just get added to the array of resources and are always displayed at the bottom of the table.

tusbar avatar Apr 17 '18 11:04 tusbar

I started working on this: https://github.com/Daniel15/BuildSize/commit/a3690cba7ed21b3f9fa1914f0b62ebdf89db7c23

However, a problem I encountered is that the default CircleCI artifacts directory ($CIRCLE_ARTIFACTS) is a temporary directory with a random name, so the full path is something like tmp/circle-artifacts.Tj1JQTO/example.txt, and it changes for every build 😕

So it seems like using the path would work if you explicitly specify it in your build. I could potentially check for a prefix of tmp/ or the presence of circle-artifacts.XXXXX and strip that out, but that seems risky.

Thoughts?

Daniel15 avatar Jul 09 '18 05:07 Daniel15

Hmm, /tmp/circle-artifacts.XXXXX/ seems pretty safe to strip, no? As long as they don’t change the names…

Though, I wouldn’t mind specifying a specific artifact path to benefit from that feature. Would you be able to retrieve it? Or would it need to be something specific like /tmp/buildsize-artifacts (default and customizable in the web interface)?

Thinking about it, it wouldn’t be a bad idea if only the artifacts in a specific path were analyzed. It would allow people to store artifacts that would be ignored by BuildSize.

tusbar avatar Jul 09 '18 11:07 tusbar

As long as they don’t change the names…

Yeah that's what I'm worried about - It's purely an implementation detail.

However, perhaps we could look at all the file paths, and strip any prefix that's common to all of them. Maybe that'll be good enough?

Thinking about this more, I think I only need to care about the path if there's multiple files with the same name. If there's only one artifact with that file name, we don't need to do much with the path. That might simplify things a bit and reduce the risk of error.

Daniel15 avatar Jul 11 '18 07:07 Daniel15

Hi @Daniel15, following up on this…

$CIRCLE_ARTIFACTS was only for CircleCI 1.0, right?

With the following config

-  store_artifacts:
     name: Store built bundles for stats
     path: bundle-stats

I’m getting the following artifacts

[ {
  "path" : "home/circleci/geodatagouv/bundle-stats/chunk-centered-map.decb662b99e9a81936ee.js.gz",
  "pretty_path" : "home/circleci/geodatagouv/bundle-stats/chunk-centered-map.decb662b99e9a81936ee.js.gz",
  "node_index" : 0,
  "url" : "https://10162-67130793-gh.circle-artifacts.com/0/home/circleci/geodatagouv/bundle-stats/chunk-centered-map.decb662b99e9a81936ee.js.gz"
}, {
  "path" : "home/circleci/geodatagouv/bundle-stats/chunk-commons.2916f0984570ddea5170.js.gz",
  "pretty_path" : "home/circleci/geodatagouv/bundle-stats/chunk-commons.2916f0984570ddea5170.js.gz",
  "node_index" : 0,
  "url" : "https://10162-67130793-gh.circle-artifacts.com/0/home/circleci/geodatagouv/bundle-stats/chunk-commons.2916f0984570ddea5170.js.gz"
} ]

The API docs say that path should be relative to the working_directory, apparently this is wrong as my working_directory is ~/geodatagouv here.

Though, we could easily strip the base path now…

tusbar avatar Oct 11 '18 09:10 tusbar

Unfortunately, I haven't had time to look at this at all :(

I wonder if we should store the full path and strip it only when displaying it.

Daniel15 avatar Oct 14 '18 19:10 Daniel15

Agreed. We should definitely store the full path.

tusbar avatar Oct 15 '18 07:10 tusbar