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

Fixed some follow ups after restructuring and issues around generator-theia #310

Merged
merged 10 commits into from
Jul 20, 2017
1 change: 1 addition & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ before_script:
before_install:
- "npm install -g npm@^4"
- "google-chrome-stable --headless --disable-gpu --remote-debugging-port=9222 http://localhost &"
bundler_args: --retry 0
Copy link

Choose a reason for hiding this comment

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

Is this travis specific or it's passed to lerna or something ?
I can't find options on lerna or npm and travis ci references bundler_args with the ruby bundler

Copy link
Member Author

Choose a reason for hiding this comment

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

It is travis specific, depending on provided language travis knows how to build the project, like run npm install first and then npm test for node projects. It also has some defaults for such projects, e.g. for node to try to run npm install 3 times if the first fails, since npmjs sometimes is not available.

Copy link

Choose a reason for hiding this comment

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

OK thanks!

notifications:
webhooks:
urls:
Expand Down
18 changes: 18 additions & 0 deletions .vscode/launch.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,24 @@
// For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387
"version": "0.2.0",
"configurations": [
{
"type": "node",
"request": "launch",
"name": "Launch Electron Backend",
"runtimeExecutable": "${workspaceRoot}/node_modules/.bin/electron",
"program": "${workspaceRoot}/examples/electron/src-gen/frontend/electron-main.js",
"protocol": "legacy",
"args": [
"--loglevel=debug"
],
"sourceMaps": true,
"outFiles": [
"${workspaceRoot}/examples/electron/src-gen/frontend/electron-main.js",
"${workspaceRoot}/examples/electron/src-gen/backend/main.js",
"${workspaceRoot}/examples/browser/lib/**/*.js",
"${workspaceRoot}/packages/*/lib/**/*.js"
]
},
{
"type": "node",
"request": "launch",
Expand Down
22 changes: 16 additions & 6 deletions doc/Developing.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ This command does a few things:

## Run the browser-based example application

We can start the application with:
We can start the application from examples/browser directory with:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest saying "from the".


npm start

Expand All @@ -50,7 +50,16 @@ tab with the frontend.

## Run the electron-based example application

It can also be started with:
From the root directory run:

npm run rebuild:electron

This command rebuilds native node packages against the version of node used by electron.
To rollback changes run:

npm run rebuild:web

It can also be started with from examples/electron directory:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd change it to

It can also be started from the examples/electron directory with:

Copy link

Choose a reason for hiding this comment

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

I don't see why someone would want to rollback however ?
Is this more confusing then helping?

Maybe move this to the web basec example section?

Copy link
Member Author

Choose a reason for hiding this comment

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

After rebuilding for electron, web won't work, but by default web is working. I can put in the web section that if you run npm run rebuild:electron don't forget to run npm run rebuild:web fisrt.

Copy link

Choose a reason for hiding this comment

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

OK yes I was curious about that so this means that since with hoisting we have only 1 node-modules we can only have 1 example at the time without rebuilding :(
But yes sounds better to me in the web section thx

Copy link
Member Author

@akosyakov akosyakov Jul 19, 2017

Choose a reason for hiding this comment

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

It could be that we can have a setup where we exclude node-pty from hoisting and don't need to run rebuild, but I've spent half a day and failed to come up with such. I think mostly we work with one or another example anyway and later with more experience maybe can overcome it.

Copy link

Choose a reason for hiding this comment

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

Sounds good


npm start

Expand Down Expand Up @@ -116,16 +125,17 @@ To build and run the browser example:

git clone https://github.com/theia-ide/theia \
&& cd theia \
&& npm install --unsafe-perm\
&& cd ../../examples/browser \
&& npm install --unsafe-perm \
Copy link

Choose a reason for hiding this comment

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

BTW we discussed this a bit and --unsafe-perm looks really weird.
I Suggest we move it out of the TL;DR and add it to the troubleshooting section
but that can be in another PR

Copy link
Member Author

@akosyakov akosyakov Jul 19, 2017

Choose a reason for hiding this comment

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

Could you open an issue for it? or PR :)

Copy link

Choose a reason for hiding this comment

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

Issue #311

&& cd examples/browser \
&& npm run start

To build and run the electron example:

git clone https://github.com/theia-ide/theia \
&& cd theia \
&& npm install --unsafe-perm\
&& cd ../../examples/electron \
&& npm install --unsafe-perm \
&& npm run rebuild:electron \
&& cd examples/electron \
&& npm run start

## Code coverage
Expand Down
14 changes: 0 additions & 14 deletions doc/Internals.md
Original file line number Diff line number Diff line change
Expand Up @@ -198,20 +198,6 @@ And it returns the loggerServer as the object that will be exposed over JSON-RPC

This connects the factory to the connection.

So now all is set for backend/frontend communication.

The only point left is that if you're using the webpack dev server which
you probably are you need to add something like this:

``` javascript
'/logger/*': {
target: 'ws://localhost:3000',
ws: true
},
```

To webpack.config.js so that the requests are proxied to the backend properly.

Copy link

Choose a reason for hiding this comment

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

We should update it to say that now the endpoints with /services/* are served by the webpack dev server

## Connecting to a service

So now that we have a backend service let's see how to connect to it from
Expand Down
12 changes: 2 additions & 10 deletions examples/browser/webpack.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ const CopyWebpackPlugin = require('copy-webpack-plugin');
const CircularDependencyPlugin = require('circular-dependency-plugin');

const outputPath = path.resolve(__dirname, 'lib');
const emptyPath = path.resolve(__dirname, 'webpack_empty.js');

const monacoEditorPath = '../../node_modules/monaco-editor-core/min/vs';
const monacoLanguagesPath = '../../node_modules/monaco-languages/release';
Expand Down Expand Up @@ -64,18 +63,13 @@ module.exports = {
resolve: {
extensions: ['.js'],
alias: {
'vs': path.resolve(outputPath, monacoEditorPath),
'dtrace-provider': emptyPath,
'safe-json-stringify': emptyPath,
'mv': emptyPath,
'source-map-support': emptyPath
'vs': path.resolve(outputPath, monacoEditorPath)
Copy link

Choose a reason for hiding this comment

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

Why is this not needed for bunyan anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't use bunyan in the frontend and use the webpack to build only the frontend, so these modules are never bundled

Copy link

Choose a reason for hiding this comment

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

Right :) thx

}
},
devtool: 'source-map',
plugins: [
// @ts-ignore
new webpack.HotModuleReplacementPlugin(),
CopyWebpackPlugin([
new CopyWebpackPlugin([
{
from: requirePath,
to: '.'
Expand Down Expand Up @@ -124,8 +118,6 @@ module.exports = {
'*': 'http://' + host + ':' + port,
},
historyApiFallback: true,
hot: true,
inline: true,
stats: {
colors: true,
warnings: false
Expand Down
Empty file removed examples/browser/webpack_empty.js
Empty file.
4 changes: 1 addition & 3 deletions examples/electron/webpack.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ module.exports = {
filename: 'bundle.js',
path: outputPath,
libraryTarget: 'umd'

},
target: 'electron',
node: {
Expand Down Expand Up @@ -64,9 +63,8 @@ module.exports = {
},
devtool: 'source-map',
plugins: [
// @ts-ignore
new webpack.HotModuleReplacementPlugin(),
CopyWebpackPlugin([
new CopyWebpackPlugin([
{
from: monacoEditorPath,
to: 'vs'
Expand Down
11 changes: 8 additions & 3 deletions generator-theia/src/common/abstract-backend-generator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,16 @@ function start() {
application.use(express.static(path.join(__dirname, '../../lib'), {
index: 'index.html'
}));
application.start();
return application.start(${this.ifWeb(`${this.model.config.port}, '${this.model.config.host}'`)})
}

Promise.resolve()${this.compileBackendModuleImports(backendModules)}
.then(start);`;
module.exports = Promise.resolve()${this.compileBackendModuleImports(backendModules)}
.then(start).catch(reason => {
console.error('Failed to start the backend application.');
if (reason) {
console.error(reason);
}
});`;
}

}
9 changes: 7 additions & 2 deletions generator-theia/src/common/abstract-frontend-generator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,13 @@ function start() {
application.start();
}

Promise.resolve()${this.compileFrontendModuleImports(frontendModules)}
.then(start);`;
module.exports = Promise.resolve()${this.compileFrontendModuleImports(frontendModules)}
.then(start).catch(reason => {
console.error('Failed to start the frontend application.');
if (reason) {
console.error(reason);
}
});`;
}

}
18 changes: 17 additions & 1 deletion generator-theia/src/common/abstract-generator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ export abstract class AbstractGenerator {
return `Promise.resolve(${invocation})`;
}
return invocation;
}).map(statement => `.then(function () { return ${statement}.then(load) })`);
}).map(statement => ` .then(function () { return ${statement}.then(load) })`);
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this change for?

Copy link
Member Author

Choose a reason for hiding this comment

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

to fix indentation in the generated source files: src-gen/frontend/main.js and src-gen/backend/main.js

Copy link

Choose a reason for hiding this comment

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

Yes thx for that! I had run into trouble with it

return os.EOL + lines.join(os.EOL);
}

Expand All @@ -60,4 +60,20 @@ export abstract class AbstractGenerator {
return copyright ? copyright + os.EOL : '';
}

protected isWeb(): boolean {
return this.model.target === 'web';
}

protected isElectron(): boolean {
return this.model.target === 'electron';
}

protected ifWeb(value: string, defaultValue: string = '') {
return this.isWeb() ? value : defaultValue;
}

protected ifElectron(value: string, defaultValue: string = '') {
return this.isElectron() ? value : defaultValue;
}

}
46 changes: 9 additions & 37 deletions generator-theia/src/common/app-package-generator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,6 @@ export class AppPackageGenerator extends AbstractGenerator {
fs.writeJSON('package.json', this.compilePackage());
if (!fs.exists('webpack.config.js')) {
fs.write('webpack.config.js', this.compileWebpackConfig());
if (this.isWeb()) {
fs.write('webpack_empty.js', '');
}
}
}

Expand All @@ -29,22 +26,22 @@ export class AppPackageGenerator extends AbstractGenerator {
"start:backend:debug": "npm run build:backend && node ./src-gen/backend/main.js --loglevel=debug | bunyan",
"start:frontend": "webpack-dev-server --open",
} : {
"postinstall": "electron-rebuild",
"start": "npm run build:backend && electron ./src-gen/frontend/electron-main.js | bunyan",
"start:debug": "npm run build:backend && electron ./src-gen/frontend/electron-main.js --loglevel=debug | bunyan",
"start:debug": "npm run build:backend && electron ./src-gen/frontend/electron-main.js --loglevel=debug | bunyan"
}
const devDependencies = this.isWeb() ? {
"webpack-dev-server": "^2.5.0"
} : {
"electron": "^1.6.11"
"electron": "^1.6.11",
"electron-rebuild": "^1.5.11"
}
return {
...this.model.pck,
"dependencies": sortByKey({
...dependendencies,
...this.model.pck.dependencies
}),
"scripts": {
"scripts": sortByKey({
"clean": "rimraf lib",
"cold:start": "npm run clean && npm start",
"build": "npm run build:frontend && npm run build:backend",
Expand All @@ -53,7 +50,7 @@ export class AppPackageGenerator extends AbstractGenerator {
"watch": "npm run build:frontend && webpack --watch",
...scripts,
...this.model.pck.scripts
},
}),
"devDependencies": sortByKey({
"rimraf": "^2.6.1",
"concurrently": "^3.5.0",
Expand All @@ -74,22 +71,6 @@ export class AppPackageGenerator extends AbstractGenerator {
}
}

protected isWeb(): boolean {
return this.model.target === 'web';
}

protected isElectron(): boolean {
return this.model.target === 'electron';
}

protected ifWeb(value: string, defaultValue: string = '') {
return this.isWeb() ? value : defaultValue;
}

protected ifElectron(value: string, defaultValue: string = '') {
return this.isElectron() ? value : defaultValue;
}

protected compileWebpackConfig(): string {
return `${this.compileCopyright()}
// @ts-check
Expand All @@ -99,8 +80,7 @@ const merge = require('webpack-merge');
const CopyWebpackPlugin = require('copy-webpack-plugin');
const CircularDependencyPlugin = require('circular-dependency-plugin');

const outputPath = path.resolve(__dirname, 'lib');${this.ifWeb(`
const emptyPath = path.resolve(__dirname, 'webpack_empty.js');`)}
const outputPath = path.resolve(__dirname, 'lib');

const monacoEditorPath = '../../node_modules/monaco-editor-core/min/vs';
const monacoLanguagesPath = '../../node_modules/monaco-languages/release';
Expand All @@ -118,8 +98,7 @@ module.exports = {
output: {
filename: 'bundle.js',
path: outputPath${this.ifElectron(`,
libraryTarget: 'umd'
`)}
libraryTarget: 'umd'`)}
},
target: '${this.model.target}',
node: {${this.ifElectron(`
Expand Down Expand Up @@ -155,18 +134,13 @@ module.exports = {
resolve: {
extensions: ['.js'],
alias: {
'vs': path.resolve(outputPath, monacoEditorPath)${this.ifWeb(`,
'dtrace-provider': emptyPath,
'safe-json-stringify': emptyPath,
'mv': emptyPath,
'source-map-support': emptyPath`)}
'vs': path.resolve(outputPath, monacoEditorPath)
}
},
devtool: 'source-map',
plugins: [
// @ts-ignore
new webpack.HotModuleReplacementPlugin(),
CopyWebpackPlugin([${this.ifWeb(`
new CopyWebpackPlugin([${this.ifWeb(`
{
from: requirePath,
to: '.'
Expand Down Expand Up @@ -215,8 +189,6 @@ module.exports = {
'*': 'http://' + host + ':' + port,
},
historyApiFallback: true,
hot: true,
inline: true,
stats: {
colors: true,
warnings: false
Expand Down
2 changes: 1 addition & 1 deletion generator-theia/src/common/generator-model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ export class Model {
for (const extension of extensions) {
const modulePath = extension[primary] || (secondary && extension[secondary]);
if (typeof modulePath === 'string') {
const extensionPath = path.join(extensionPackage.name, modulePath);
const extensionPath = path.join(extensionPackage.name, modulePath).split(path.sep).join('/')
result.set(`${primary}_${moduleIndex}`, extensionPath);
moduleIndex = moduleIndex + 1;
}
Expand Down
13 changes: 6 additions & 7 deletions generator-theia/src/electron/electron-frontend-generator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,21 +28,20 @@ process.env.LC_NUMERIC = 'C';
const electron = require('electron');
const path = require('path');

let mainWindow = undefined;

electron.app.on('window-all-closed', function () {
if (process.platform !== 'darwin') {
electron.app.quit();
}
});

electron.app.on('ready', function () {
require("../backend/main");
mainWindow = new electron.BrowserWindow({ width: 1024, height: 728 });
mainWindow.webContents.openDevTools();
mainWindow.loadURL(\`file://\${path.join(__dirname, '../../lib/index.html')}\`);
const mainWindow = new electron.BrowserWindow({ width: 1024, height: 728 });
require("../backend/main").then(server => {
mainWindow.loadURL(\`file://\${path.join(__dirname, '../../lib/index.html')}?port=\${server.address().port}\`);
mainWindow.webContents.openDevTools();
});
mainWindow.on('closed', function () {
mainWindow = undefined;
electron.app.exit(0);
});
});`;
}
Expand Down
Loading