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

Reorganize bundles and buildles #6299

Closed
2 tasks
brendankenny opened this issue Oct 17, 2018 · 12 comments · Fixed by #6344
Closed
2 tasks

Reorganize bundles and buildles #6299

brendankenny opened this issue Oct 17, 2018 · 12 comments · Fixed by #6344

Comments

@brendankenny
Copy link
Member

brendankenny commented Oct 17, 2018

Originally we had only popup.js, which split off into the lighthouse-background.js extension entry point, which then also hosted a DevTools entry point, which split off into its own file that the extension entry point extended, then LR lived in there and then also split off, and then finally all the entry files were independent.

In all this time, they've all lived in lighthouse-extension/app/src/, and when built they all ended up in lighthouse-extension/dist/scripts/, even though 2/3 of them now have nothing to do with the extension except they share a build script.

#6295 splits building those files and building the extension, which means we can now do a clean reorg of those files without much fuss. Ideally the result would be easily navigable and obvious in its purpose, with no weirdness based on git history that's long disappeared.

Here's my suggestion, but I don't find it completely satisfying and I'm not particularly wedded to the structure or names.

├── clients/
│   ├── devtools-entry.js   (could be in subdirectory but seems unnecessary)
│   ├── lightrider-entry.js
│   ├── extension/   (current top-level lighthouse-extension/)
│   |   ├── scripts/ (currently src/)
│   |   ├── styles/
│   |   ├── manifest.json
│   |   ├── popup.html
│   |   └── etc...
│   └── test/
│       ├── lightrider-entry-test.js
│       └── extension/
│          ├── extension-test.js
│          └── popup-test.js
├── build/
│   ├── build-extension.js
│   ├── bundle-builder.js
│   └── changelog-generator/   (already exists)
├── dist/   (created while building)
│   ├── lighthouse-dt-bundle.js
│   ├── lighthouse-lr-bundle.js
│   ├── extension/
│   |   └── scripts/, styles/, manifest.json, etc
│   └── extension-package
│          ├── lighthouse-3.2.0.zip
│          └── lighthouse-3.2.1.zip
├── lighthouse-cli/
├── lighthouse-core/
└── etc...

"bundles/" is a dumb name. clients/? channels/? It's nice to describe that they are clients/channels, but they only work when bundled and then run somewhere else. (now using clients/)

That aside, it makes sense to have the entry points in a top-level directory (similar to cli/), but also makes sense to group them together because they aren't independent, executable entry points into the module itself (unlike the CLI, they're only useful after building).

It would also be great to move to a top-level output of the build step. out/ is popular in some parts of the world, but we can take our pick.

Thoughts?

more requirements:

  • logging of build steps
  • browserify without minifying option?
@brendankenny
Copy link
Member Author

@paulirish prefered clients/. Updated

@paulirish
Copy link
Member

👍 I like it.

@patrickhulce
Copy link
Collaborator

I like it too 👍

I'm partial to channels/ also throwing consumers/ out there. clients/ just makes me think of something very different given the LR ambitions and discussions of a translator library/API client.

In JS-land I feel like I've seen dist/ more than out/ but I'm not married to anything

@connorjclark
Copy link
Collaborator

connorjclark commented Oct 17, 2018

I have never seen out/, but dist/ is super common. But I believe it's normally in a slightly different context - in most projects that I've seen with dist/, the entire folder is published to npm (so I guess it's short for distributables). This out/ folder, I guess, contains multiple distributions. An insignificant difference. It's functionally the same, just more than one thing to distribute.

@brendankenny
Copy link
Member Author

I assume out/ originally came from a.out? It's at least somewhat common (e.g. Chromium uses it by default). I personally dislike dist/, but I also don't care much about it :)

But I believe it's normally in a slightly different context - in most projects that I've seen with dist/, the entire folder is published to npm

good point. In our case the root directory is our npm package, while this directory is going to be specifically npmignored

@patrickhulce
Copy link
Collaborator

I personally dislike dist/, but I also don't care much about it :)

I'm so curious, why no ❤️ for dist? :)

@brendankenny
Copy link
Member Author

brendankenny commented Oct 17, 2018

I'm so curious, why no ❤️ for dist? :)

ha, it's probably all on me. My first thought when I see dist in code is, "did you mean dest?". Then I remember it's common so is probably on purpose, and then sit there trying to remember what it's short for.

Seriously, almost every time I see it :)

@wardpeet
Copy link
Collaborator

I like consumers and dist folders 🤷‍♂️

I'm not 100% sure of the build-extension name. If we call the folder clients, shouldn't we call it build-client.js or bundle-client.js? Extension seems wrong.
├── build/
│ ├── build-extension.js
│ ├── bundle-builder.js

@brendankenny
Copy link
Member Author

build-extension.js does more than bundling from extension-entry.js (really bundle-builder.js or whatever does that part). build-extension.js also copies over extension assets, preps popup.js, and creates the crx zip.

But we can also pick a better name :)

@wardpeet
Copy link
Collaborator

build-client.js 🙃 ?

The reason why I think extension is not a correct name is because IMO it tells me that it builds the chrome extension. That's the only reason why I would replace "extension"

function buildAll() {
  return Object.values(CONSUMERS).map(consumer => {
    const inFile = `app/src/${consumer.src}`;
    const outFile = `dist/scripts/${consumer.dist}`;
    return bundleBuilder.build(inFile, outFile);
  });
}

@brendankenny
Copy link
Member Author

Oh, sorry, I see now. Yes, I would take the others out and have it only build the extension (much simpler that way). The other clients would be built elsewhere (pretty much a one liner each)

@brendankenny
Copy link
Member Author

switched to dist/ and added some more requirements that came up in review of #6295

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 a pull request may close this issue.

5 participants