Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

(WIP) [Toolbox] Decouple the development environment #1181

Merged
merged 1 commit into from Nov 15, 2016

Conversation

jasonLaster
Copy link
Contributor

@jasonLaster jasonLaster commented Nov 13, 2016

Associated Issue: #1156

This is some preliminary work needed to get other tools to work with the local toolbox. This work came out of hacking on console.html while on the plane.

There are some high level pieces to look out for:

  1. each tool has a bin/dev-server that uses the toolbox's dev-server
  2. each tool has a configs directory
  3. each tool has a webpack.config that uses the toolbox's config.
  4. each tool is responsible for reading configs and then passing them along to services (server, tests, webpack config). This is a simpler system than before.
  5. the toolbox index.js and client/index.js is tool agnostic. This includes the console passing in a dom element as root (not a react component).
  6. I added some devtools-modules needed for the console

Screenshot

const serveIndex = require("serve-index");
const express = require("express");

const toolbox = require("../node_modules/devtools-local-toolbox/index");
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just do require("devtools-local-toolbox") ?

Choose a reason for hiding this comment

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

Help fix my duplicate account

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's a confusing aspect at the moment.

The reason for this is that devtools-local-toolbox exports src/index.js not index.js

This will be cleaned up in a subsequent PR, but it's not related to the console working per se. The issue here is that the toolbox does 2 things:

  1. It sets up a development environment - dev server and webpack config
  2. It has a web app for the landing page and establishing the connection

The development environment does not belong in the web app's webpack bundle so it's in its own index. I'll move it to a separate package after this is done.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the main thing was referencing node_modules directly though: you should be able to do require("devtools-local-toolbox/index") right?

@@ -0,0 +1,20 @@
const _ = require("lodash");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's a little confusing for code and configs to share the same directory. I'm still getting my head around this new structure so I don't know what to propose yet, but ideally the top-level config directory would just contain json files.

Copy link

@Pomax Pomax Nov 15, 2016

Choose a reason for hiding this comment

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

also when using lodash in node code, it generally makes more sense to call the variable itself lodash, not _, since it's much for informative to grep for a full word string like lodash than it is to grep for a single character _ (especially with Node's ___filename and __dirname constants).

That said, most of what lodash does is covered by ES6+ these days, so that the only thing it's being used for in this code is covered by vanilla JS these days as Object.assign({}, envConfig, localConfig);

Copy link
Contributor

Choose a reason for hiding this comment

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

@Pomax I agree. I've advocated for limiting our use of lodash. I would suggest removing it from this file as well.

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 think config.js in the top level would probably be fine.

I'm hesitant to move it into src or bin as those are different in my mind.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is src code so I don't see why src wouldn't make sense?

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'm okay using ES6 where we can, but in this case, we do need a deep merge, so lodash.merge makes sense to me

@@ -10,8 +10,12 @@
"url": "https://github.com/devtools-html/debugger.html/issues"
},
"homepage": "https://github.com/devtools-html/debugger.html#readme",
"engineStrict": true,
"engines": {
"node": ">=6.4.0"
Copy link

Choose a reason for hiding this comment

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

given the LTS status of 6.9, probably worth bumping this to >=6.9.0

@jasonLaster jasonLaster force-pushed the console-html branch 2 times, most recently from 3ef3466 to 35cad0e Compare November 15, 2016 22:09
@jasonLaster jasonLaster merged commit f183b11 into firefox-devtools:master Nov 15, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants