★ wanayoo — archive 1999 https://github.com/firefox-devtools/debugger/pull/6220Nouvelle recherche | Portail wanayoo
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

[WIP] Transpile debugger modules for DevTools loader #6220

Merged
merged 6 commits into from May 7, 2018

Conversation

@juliandescottes
Copy link
Member

@juliandescottes juliandescottes commented May 4, 2018

This is a work in progress PR, intended for collaboration and discussion, do not merge!

cc @jasonLaster @ochameau

I started doing some cleanup, and rebased on latest master.

In my opinion, TODOs:

@jasonLaster
Copy link
Contributor

@jasonLaster jasonLaster commented May 4, 2018

find alternative to workaround in CallSite.js

could we just do import "callsite.css" basically what we do everywher?

@juliandescottes juliandescottes force-pushed the juliandescottes:mc-babel branch 2 times, most recently from 3fb717a to b19b45b May 4, 2018
@juliandescottes
Copy link
Member Author

@juliandescottes juliandescottes commented May 4, 2018

Applied Jason's suggestion for Callsite.css.
Cleaned up transform-mc and copy-modules.
Mutualized the m-c mappings between webpack-config and copy-modules/transform-mc

ModuleDeclaration(path, state) {
const source = path.node.source;
const value = source && source.value;
if (value && value.includes(".css")) {

This comment has been minimized.

@juliandescottes

juliandescottes May 4, 2018
Author Member

@jasonLaster I don't remember exactly the purpose of this part, can you comment?

@@ -0,0 +1,30 @@
/* This Source Code Form is subject to the terms of the Mozilla Public

This comment has been minimized.

@juliandescottes

juliandescottes May 4, 2018
Author Member

@jasonLaster I put this under the root right now, maybe we should have a firefox-integration folder or something?

if (isDevelopment()) {
// In local development, use the debugger as a single bundle
webpackConfig.entry.debugger = getEntry("src/main.js");
} else {

This comment has been minimized.

@juliandescottes

juliandescottes May 4, 2018
Author Member

@jasonLaster When we are not in development, we no longer build the debugger bundle, but we still build other bundles. Maybe we can simplify some things here?

@darkwing darkwing added the 📚 pr-wip label May 4, 2018
@juliandescottes
Copy link
Member Author

@juliandescottes juliandescottes commented May 4, 2018

The DebuggerConfig issue should be resolved by #6223, so marking this one as resolved. Still mochitests to fix now!

@juliandescottes
Copy link
Member Author

@juliandescottes juliandescottes commented May 5, 2018

Totally forgot I had to whitelist a few duplicate items:

https://hg.mozilla.org/try/comparison/9b6fdd663985/browser/installer/allowed-dupes.mn

They are all types! And all simply contain "use strict". We should probably not transpile/copy/package them!

@juliandescottes
Copy link
Member Author

@juliandescottes juliandescottes commented May 5, 2018

There is also a new dupe for the chevron:

browser/chrome/devtools/skin/images/command-chevron.svg
browser/chrome/devtools/skin/images/debugger/command-chevron.svg
@juliandescottes juliandescottes force-pushed the juliandescottes:mc-babel branch from b19b45b to c85cb06 May 5, 2018
@juliandescottes juliandescottes requested a review from flodolo as a code owner May 5, 2018
@juliandescottes
Copy link
Member Author

@juliandescottes juliandescottes commented May 5, 2018

Mochitests should now be fixed: the issue was with build-query.js where I earlier changed:

- import escapeRegExp from "lodash/escapeRegExp";
+ import { escapeRegExp } from "lodash";

This worked well when the file is used in the context of the debugger, but it is also used in a worker. In this case, from "lodash" was triggering the usage of an m-c mapping, trying to require lodash from our devtools vendors folder, which of course doesn't work in a worker.

First comment, we probably should not use the mc-mappings when bundling workers, they won't be able to require them anyway. And second comment, since those workers will need to pull and bundle everything we should be careful not to bundle huge libraries with them.

The solution here is to handle the special case of "lodash/subModule" requires in our babel plugin.

@juliandescottes
Copy link
Member Author

@juliandescottes juliandescottes commented May 5, 2018

Hmmmpf I rebased this on release 46 in order to make sure to have a clean baseline for tests, which is making the whole PR unreadable

@juliandescottes juliandescottes changed the base branch from master to release-46 May 5, 2018
@juliandescottes juliandescottes removed the request for review from flodolo May 5, 2018
@juliandescottes juliandescottes force-pushed the juliandescottes:mc-babel branch from cc264c5 to ee4551f May 5, 2018
@juliandescottes
Copy link
Member Author

@juliandescottes juliandescottes commented May 5, 2018

Excluded types from copy modules.

@juliandescottes
Copy link
Member Author

@juliandescottes juliandescottes commented May 5, 2018

Here is a try run with the latest version, this one should hopefully be green:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b39504b755ddb7eacb2282ad9fc0091217a9ea8

Edit: Yay! Green try!

@juliandescottes juliandescottes force-pushed the juliandescottes:mc-babel branch from ee4551f to 698e632 May 5, 2018
@juliandescottes
Copy link
Member Author

@juliandescottes juliandescottes commented May 5, 2018

The travis build is failing right now because it can't see the files transferred via copy modules. This is most likely because we need to build again after copy-modules

@juliandescottes
Copy link
Member Author

@juliandescottes juliandescottes commented May 5, 2018

All my TODOs are addressed, we have a green try, a green travis. We can move to the review step!

@juliandescottes juliandescottes requested a review from jasonLaster May 5, 2018
@jasonLaster
Copy link
Contributor

@jasonLaster jasonLaster commented May 5, 2018

This looks great!

2 things:

  1. do you think we can land this on master?
  2. do you think the mappings file could go in the config dir?
@jasonLaster
Copy link
Contributor

@jasonLaster jasonLaster commented May 5, 2018

also - one other thing that might be a nice follow up, but I've been thinking about the directory structure and i think this might be more future forward:

/new 
  /dist
     parser-worker.js
     search-worker.js
     vendors.js
  /actions
  /components
  /...
  /test
  index.html
  panel.js
  README.md

the benefits are:

  1. the src files are similar to other panels
  2. the bundles are in a directory that hopefully will go away over time

I think we could get this directory structure by tweaking copy-assets and copy-modules. Again, we could do this as a second step...

jasonLaster and others added 3 commits Apr 25, 2018
- generate vendors.js only when isDevelopment() is false
- generate debugger.js only when isDevelopment() is true
- standardize vendors.js and add comments
- add temporary workarounds to enable transpiled version to work
@juliandescottes juliandescottes force-pushed the juliandescottes:mc-babel branch from 175664b to 820daf1 May 7, 2018
@juliandescottes juliandescottes changed the base branch from release-46 to master May 7, 2018
@juliandescottes
Copy link
Member Author

@juliandescottes juliandescottes commented May 7, 2018

Rebased + moved mozilla-central-mappings.

the src files are similar to other panels

We actually have src/ in netmonitor and application panel. While discussing it before creating the application panel, this seemed like the less messy option, although we haven't tried yet to refactor other panels to do the same. So I would rather keep src for now here.

the bundles are in a directory that hopefully will go away over time

This sounds like a good step. I'll try to fit it in here.

const mcDebuggerPath = path.join(mcPath, "devtools/client/debugger/new");

console.log("[copy-modules] copying files to: " + mcDebuggerPath);
shell.cp("-r", "./out/src", mcDebuggerPath);

This comment has been minimized.

@jasonLaster

jasonLaster May 7, 2018
Contributor

it would be nice to rm ./out when we're done.

ModuleDeclaration(path, state) {
const source = path.node.source;
const value = source && source.value;
if (value && value.includes(".css")) {

This comment has been minimized.

@jasonLaster

jasonLaster May 7, 2018
Contributor

we need to remove the CSS imports e.g. import "CallSite.css"

@jasonLaster jasonLaster merged commit 4ce2953 into firefox-devtools:master May 7, 2018
1 of 3 checks passed
1 of 3 checks passed
ci/circleci Your tests are queued behind your running builds
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
deploy/netlify Deploy preview ready!
Details
juliandescottes added a commit that referenced this pull request May 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants