-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Changes from 4 commits
da86895
0f8189c
b87dccd
e70a143
64c9ddc
29b824c
d2cf122
2481dfe
52a0dfe
fef2693
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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: | ||
|
||
npm start | ||
|
||
|
@@ -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: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd change it to
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see why someone would want to rollback however ? Maybe move this to the web basec example section? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 :( There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good |
||
|
||
npm start | ||
|
||
|
@@ -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 \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BTW we discussed this a bit and --unsafe-perm looks really weird. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you open an issue for it? or PR :) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) })`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is this change for? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. to fix indentation in the generated source files: There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
|
||
|
@@ -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; | ||
} | ||
|
||
} |
There was a problem hiding this comment.
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".