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

Initial work for enabling React Native Intellisense #9

Merged
merged 15 commits into from
Feb 9, 2016

Conversation

joshuaskelton
Copy link
Contributor

Summary

Enabling Salsa intellisense for React Native. This is done by configuring the workspace as outlined here: https://github.com/Microsoft/vscode-docs/blob/vnext/release-notes/latest.md

Additionally we configure the tsconfig.json file which is not mentioned in the above, but still needed.

Changes

  • Ported the TsdHelper from vscode-cordova
  • Ported needed file system helper functions
  • Created a TsConfigHelper to help work with Typescript configuration files
  • Added typings for React and React Native

/**
* Helper function to asynchronously copy a file
*/
public static copyFile(from: string, to: string, encoding?: string): Q.Promise<any> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to open the files in binary format so we won't need to pass the encoding parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's an optional parameter and we don't actually use it. I still think it is useful even if we currently aren't using it. If you feel strongly about it, I would consider removing it.

## Changes

- Made TsConfigHelper promisey
- Fixed promises in TsdHelper
- Simplified how we work with the listing of typings
var fileSystem:FileSystem = new FileSystem();
var reactTypeDefsPath = path.resolve(__dirname, "..", "..", "reactTypings.json");

if (fileSystem.existsSync(reactTypeDefsPath)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to check if this file exists? Dont we want the extesion to throw an error if its not there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we want to throw an error?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming that this file will be shipped with the extension. The only reason why it might not be there might be because the installation of the extension broke somehow. I'm guessing that we want to show an error, and possibly suggest the person to re-install the extension (so this file will appear on the proper place).

@dlebu
Copy link
Contributor

dlebu commented Feb 5, 2016

Minor comments, otherwise LGTM!

*/
public makeDirectoryRecursive(dirPath: string): void {
let parentPath = path.dirname(dirPath);
if (!this.existsSync(parentPath)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In general, we should try to make anything that runs inside the extension process Async because we share that process with other extensions.

// Add typings for React and React Native
var reactTypeDefsPath = path.resolve(__dirname, "..", "reactTypings.json");
var typeDefsToInstall:string[] = require(reactTypeDefsPath);
TsdHelper.installTypings(TsdHelper.getOrCreateTypingsTargetPath(vscode.workspace.rootPath),typeDefsToInstall).done();
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

# Conflicts:
#	src/rn-extension.ts
## Changes
- Removed TsdHelper
- Added copyRecursive to fileSystem
- Added environment variable check to lighting up intellisense
@@ -0,0 +1,14 @@
[
"react/react.d.ts",
Copy link
Member

Choose a reason for hiding this comment

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

why do we need this json file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't I'll remove it.

@nisheetjain
Copy link
Member

Looks good. :)

joshuaskelton added a commit that referenced this pull request Feb 9, 2016
Initial work for enabling React Native Intellisense
@joshuaskelton joshuaskelton merged commit fe3cb64 into master Feb 9, 2016
@joshuaskelton joshuaskelton deleted the react-native-intellisense branch February 9, 2016 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants