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

Improve the workspace #179

Closed
wants to merge 3 commits into from
Closed

Improve the workspace #179

wants to merge 3 commits into from

Conversation

rianby64
Copy link
Collaborator

  • Remove an unnecessary dependency: "devtools-terminal".
  • Add the support for .es extensions to VSCode

Motivation and Context

devtools-terminal is not compatible with linux environments. If run npm install then a bunch of errors will appear and this fact obstructs the development in other platforms different to MacOS.

How Has This Been Tested?

After removing that dependency, I ran all the scripts and all of them didn't fail. Looks like that dependecy can be safely dropped.

- Remove an unnecessary dependency: "devtools-terminal".
- Add the support for .es extensions to VSCode
@@ -5,3 +5,30 @@ Scripts used for configuring server nodes
## `nginx`

nginx setup. Derived from [`whatwg/misc-server`](https://github.com/whatwg/misc-server)

## `vscode`
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this section necessary to add in addition to the config file?

if so, does it really belong in the main README or could it go into a separate doc page?

Copy link
Member

@snuggs snuggs Dec 11, 2018

Choose a reason for hiding this comment

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

Hmmm @brandondees @rianby64 you both are right. It IS part of "configuration" per say. The thought was anything "configure" related stuff goes in /configure. But that is more internal dev related (perhaps shouldn't be?). Really was just a place I could quarantine the "This is how you get nginx to work for WHATWG" code I got from WHATWG/meta when we did nginx.

IMHO this documentation should be as close to introduction as possible. Perhaps an IDE section in main README.md? /cc @tmornini

In addendum i'll add what I did for VIM.

Thanks for this @rianby64

Copy link
Member

@snuggs snuggs left a comment

Choose a reason for hiding this comment

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

@rianby64 please see comments.

package.json Outdated
@@ -36,7 +36,7 @@
},
"scripts": {
"test": "bin/test",
"start": "bin/serve",
"start": "bin/browse",
Copy link
Member

Choose a reason for hiding this comment

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

I think we should leave this as serve as Heroku would blowup with browser sync.

@@ -57,7 +57,6 @@
"postcss-cli": "*",
"autoprefixer": "*",
"browser-sync": "*",
"devtools-terminal": "*",
Copy link
Member

Choose a reason for hiding this comment

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

@rianby64 Also have updated some dependencies due to security risk. Check for merging of #181 which may cause (easy) conflict that may need fixing. But DEFINITELY want to get rid of this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Bad news. If you rely on "devtools-terminal" then please, check the following:
https://github.com/chjj/pty.js#todo

Add a way of determining the current foreground job for platforms other than Linux and OSX/Darwin.

devtools-terminal relies on pty which works only on Mac.

Copy link
Member

Choose a reason for hiding this comment

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

@rianby64 EXACTLY. Let's kill it with fire. To be honest it only was around when I was testing VIM in browser. I'm going to go ahead and merge this.

"files.associations": {
"*.es": "javascript"
}
```
Copy link
Member

Choose a reason for hiding this comment

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

EASY PEASY @rianby64. This is great. Now where it should go see above. But wherever it goes this is great.

package.json Outdated
"start": "bin/browse",
"package": "bin/package",
"postinstall": "bin/publish"
"test": "bin/snuggsi test",
Copy link
Member

Choose a reason for hiding this comment

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

@brandondees @rianby64 @tmornini wow can resolve merge conflicts from Github now. :-)

@rianby64 took care of it and merged into your branch as it was my doing from merging #175 earlier today.

@snuggs
Copy link
Member

snuggs commented Dec 11, 2018

@rianby64 yeah man we gotta get rid of devtools.
capture d ecran 2018-12-10 a 21 59 28

@snuggs
Copy link
Member

snuggs commented Dec 13, 2018

@rianby64 please see #183

@snuggs
Copy link
Member

snuggs commented Dec 13, 2018

@brandondees @rianby64 peep game... >>> microsoft/vscode#62976

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants