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

tsconfig.json basic support #323

Merged
merged 14 commits into from
Sep 20, 2016
Merged

tsconfig.json basic support #323

merged 14 commits into from
Sep 20, 2016

Conversation

lgrignon
Copy link
Contributor

@lgrignon lgrignon commented Jul 9, 2016

  • Checkbox "use tsconfig" in project's TypeScript preference page
  • Force reload button in case automatic reloading doesn't work
  • Overrides project preferences for supported properties coming from tsconfig file

This is pretty minimalist, but I plan to add the following feature during the days:

  • Either disable preferences coming from tsconfig.json in preferences pages OR write to tsconfig if preferences are modified in GUI
  • Reload preferences if tsconfig.json file changed
  • Support the files property, which includes glob & input files list

Hope you will like it :)

@tomshen
Copy link
Contributor

tomshen commented Jul 12, 2016

Thanks for the contribution! I'll try to get someone to take a look at this by the end of the week.

@lgrignon
Copy link
Contributor Author

Thanks. I will try to add the support for files (include / exclude) list (which is IMO one of the main points) ASAP, in the same PR if possible.

@@ -79,4 +80,7 @@
String SYNTAX_COLORING_PUNCTUATION_COLOR = "syntaxColoring.punctuation.color";
String SYNTAX_COLORING_REG_EXP_LITERAL_COLOR = "syntaxColoring.regExpLiteral.color";
String SYNTAX_COLORING_STRING_LITERAL_COLOR = "syntaxColoring.stringLiteral.color";

String PREFERENCE_STORE_TS_CONFIG_LAST_MODIFICATION_TIME = "preferenceStore.tsConfigLastModTime";

Choose a reason for hiding this comment

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

These constants look like they are alpha-ed. Both within blocks and across blocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excuse me, what do you mean by alpha-ed? My english may be not perfect :)

Choose a reason for hiding this comment

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

oh I mean alphabetical. P before S in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, no problem

@markwongsk
Copy link

Couple of things:

  1. I don't think "enable project specific settings" should be checked if "use tsconfig" is checked. I think the former should be disabled in this case.
  2. Specifying source folders don't seem to be working.
  3. It doesn't seems like forcing a full build reverts to the initial values. Here's an example trace:
Thread [Worker-21] (Suspended (breakpoint at line 248 in CompilerOptions))  
    CompilerOptions.fromProject(IProject) line: 248 
    LanguageEndpoint.initializeProjectTree(IProject, Set<IProject>) line: 317   
    LanguageEndpoint.initializeProject(IProject) line: 77   
    TypeScriptBuilder.fullBuild(IProgressMonitor) line: 127 
    TypeScriptBuilder.build(int, Map<String,String>, IProgressMonitor) line: 91 
    BuildManager$2.run() line: 734  
    SafeRunner.run(ISafeRunnable) line: 42  
    BuildManager.basicBuild(int, IncrementalProjectBuilder, Map<String,String>, MultiStatus, IProgressMonitor) line: 205    
    BuildManager.basicBuild(IBuildConfiguration, int, String, Map<String,String>, IProgressMonitor) line: 329   
    BuildManager.build(IBuildConfiguration, int, String, Map<String,String>, IProgressMonitor) line: 404    
    Project$1.run(IProgressMonitor) line: 556   
    Workspace.run(IWorkspaceRunnable, ISchedulingRule, int, IProgressMonitor) line: 2241    
    Project.internalBuild(IBuildConfiguration, int, String, Map<String,String>, IProgressMonitor) line: 534 
    Project.build(int, String, Map<String,String>, IProgressMonitor) line: 119  
    Builders$1.run(IProgressMonitor) line: 103  
    Worker.run() line: 55   

Notice that CompilerOptions#fromProject has the following first lines:

    public static CompilerOptions fromProject(IProject project) {
        checkNotNull(project);

        IPreferenceStore preferenceStore = new ProjectPreferenceStore(project);
        ...
    }

In particular, the preferenceStore is initialized without reading the tsconfig file. I think that's why I'm seeing a full build reverting to the default values.

@lgrignon
Copy link
Contributor Author

Thanks for your code review. There is a bunch of things. I will take a closer look at it ASAP.
I am nearly finished with the files / include / exclude support (without glob, like you asked me).

My next push will include your feedbacks and this "files" part.

Thanks again.

@lgrignon
Copy link
Contributor Author

Just pushed files/include/exclude part. I start today to take your feedbacks in account.
Thanks.

@lgrignon
Copy link
Contributor Author

About your last comment:

  1. I think it is a larger subject, we need to disable in the UI the preferences overriden by tsconfig. That being said, we could simplify the problem by just force uncheck "Project specific settings" to disable all inputs, but IMO, it makes more sense to check Project specific settings while using a project's tsconfig
  2. I disabled source folder input in BuildPathPreferencePage, so only files/include/exclude properties control source folders
  3. I fixed several bugs, I hope yours will be too with this commit :)

Thanks again for your time.

@glen-84
Copy link

glen-84 commented Jul 20, 2016

without glob, like you asked me

Why not? 😞

@lgrignon
Copy link
Contributor Author

cf this comment #323 (comment)

@glen-84
Copy link

glen-84 commented Jul 20, 2016

Okay, I'll have to wait for that then.

Thanks for working on this, @lgrignon =)

@tomshen
Copy link
Contributor

tomshen commented Jul 27, 2016

@markwongsk Could you take another look at this?

@markwongsk
Copy link

I'm getting the following:

java.lang.RuntimeException: The following request caused an error to be thrown:
{"endpoint":"language","method":"getAllDiagnostics","arguments":["composer-app"]}
TypeError: Cannot convert undefined or null to object
    at LanguageEndpoint.getAllDiagnostics (/Users/mwong/github/eclipse-typescript/com.palantir.typescript/bin/bridge.js:54990:20)
    at Main.processRequest (/Users/mwong/github/eclipse-typescript/com.palantir.typescript/bin/bridge.js:55243:37)
    at Interface.<anonymous> (/Users/mwong/github/eclipse-typescript/com.palantir.typescript/bin/bridge.js:55232:23)
    at emitOne (events.js:96:13)
    at Interface.emit (events.js:188:7)
    at Interface._onLine (readline.js:224:10)
    at Interface.<anonymous> (readline.js:357:12)
    at Array.forEach (native)
    at Interface._normalWrite (readline.js:356:11)
    at Socket.ondata (readline.js:96:10)
    at com.palantir.typescript.services.Bridge.processRequest(Bridge.java:142)
    at com.palantir.typescript.services.Bridge.call(Bridge.java:95)
    at com.palantir.typescript.services.language.LanguageEndpoint.getAllDiagnostics(LanguageEndpoint.java:113)
    at com.palantir.typescript.TypeScriptBuilder.createMarkers(TypeScriptBuilder.java:330)
    at com.palantir.typescript.TypeScriptBuilder.build(TypeScriptBuilder.java:195)
    at com.palantir.typescript.TypeScriptBuilder.fullBuild(TypeScriptBuilder.java:128)
    at com.palantir.typescript.TypeScriptBuilder.build(TypeScriptBuilder.java:90)
    at org.eclipse.core.internal.events.BuildManager$2.run(BuildManager.java:734)
    at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:42)
    at org.eclipse.core.internal.events.BuildManager.basicBuild(BuildManager.java:205)
    at org.eclipse.core.internal.events.BuildManager.basicBuild(BuildManager.java:329)
    at org.eclipse.core.internal.events.BuildManager.build(BuildManager.java:404)
    at org.eclipse.core.internal.resources.Project$1.run(Project.java:556)
    at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:2241)
    at org.eclipse.core.internal.resources.Project.internalBuild(Project.java:534)
    at org.eclipse.core.internal.resources.Project.build(Project.java:119)
    at com.palantir.typescript.Builders$1.run(Builders.java:103)
    at org.eclipse.core.internal.jobs.Worker.run(Worker.java:55)

With the following tsconfig:

{
    "compilerOptions": {
        "jsx": "react",
        "module": "commonjs",
        "noImplicitAny": true,
        "outDir": "build/tmp",
        "removeComments": false,
        "sourceMap": true,
        "target": "ES5"
    },
    "exclude": [
        "build",
        "node_modules",
        "test"
    ],
    "compileOnSave": false,
    "atom": {
        "rewriteTsconfig": false
    }
}

I haven't dug more into it yet.

@markwongsk
Copy link

my suspicion is that the tsconfig doesn't specify sources (files).

@markwongsk
Copy link

Okay I cannot get this to work on my project without the stacktrace above. Do you have a sample project somewhere I can try out for which your tsconfig works?

@adidahiya
Copy link

tagging @lgrignon ^

@lgrignon
Copy link
Contributor Author

lgrignon commented Jul 29, 2016

Hi. I will take a look at this, it "works on my machine" with Eclipse Neon without error. Did you try specifying files or include property? It is supposed to fallback on all project's files if not specified but maybe there is a mistake.

Another interesting information is that I test with right click > Debug As > Eclipse Application.

I will try with your tsconfig right now.

Thanks

@lgrignon
Copy link
Contributor Author

lgrignon commented Jul 29, 2016

OK I did not reproduced your bug yet, but there is some problem with compileOnSave: false

In TypeScriptBuilder.build(Set, IProgressMonitor), the following if prevents compilation, even with Project > Clean. I have never set compile on save in the past (even without tsconfig) but I suspect there is a problem with it:

private void build(Set<FileDelta> fileDeltas, IProgressMonitor monitor) throws CoreException {
        IPreferenceStore projectPreferenceStore = new ProjectPreferenceStore(this.getProject());

        this.deleteAllMarkers();

        // compile the source files if compile-on-save is enabled
        if (projectPreferenceStore.getBoolean(IPreferenceConstants.COMPILER_COMPILE_ON_SAVE)) {

I think it should rather be in incrementalBuild, right?

@lgrignon
Copy link
Contributor Author

I just reproduced your bug. I fixed it and pushed it.

Just in case, a link to a working project with your tsconfig:
https://we.tl/OkEPpjrNxv

Works with the following tsconfig as well. Thanks again for your time :)

{
    "tt": {},
    "include": [
        "ts/inner-lib/src/main",
        "ts/inner-lib/src/typings",
        "ts/my.ts",
        "ts/my2.ts"
    ],

    "exclude": [
    ],
    "compileOnSave": true,
    "compilerOptions": {
        "outDir": "bulubulup",
        "target": "es6",
        "module": "none",
        "sourceMap": false,
        "moduleResolution": "Classic",
        "emitDecoratorMetadata": true,
        "experimentalDecorators": false
    }
}

@tomshen
Copy link
Contributor

tomshen commented Aug 8, 2016

@markwongsk, could you try this out again?

@lgrignon
Copy link
Contributor Author

lgrignon commented Sep 6, 2016

Hello EclipseTypescript's team, any update on this? :)

Thanks

@markwongsk
Copy link

Hey @lgrignon I was busy and then went on vacation so didn't get to look at this. I'll be taking a look at this this week. Unfortunately, your link has expired. Would you mind updating it again? Thanks!

@markwongsk
Copy link

@lgrignon I can't get specifying source folders to work. Example repo: https://github.com/markwongsk/test-eclipse-typescript-tsconfig

Steps:

  1. Right-click project -> properties
  2. Typescript -> Source folder(s) -> type "src;typings/browser;"
  3. Click apply
  4. Right-click project -> properties. Expect: Source folders are still specified because the "Use tsconfig" checkbox is not checked. Actual, source folders are gone.

@lgrignon
Copy link
Contributor Author

Hello, sorry I did not answer earlier. Here is the link to the project: https://we.tl/E8zIx0YEZl

Indeed, there is this very stupid bug, I will fix it ASAP. Maybe you could try the tsconfig part in the mean time.

Thanks for testing / reporting.

@markwongsk
Copy link

I tried with a simple tsconfig.json and things seem to be fine. Good job. I may look into the linked project later, but for now I think it looks good other than the bug I found.

@lgrignon
Copy link
Contributor Author

Thanks a lot!

About the bug, I had a hard time trying to debug what is happening and it seems that PreferenceStore.setValue(SRC_DIRS.., "src;...") isn't stored and I didn't figure out why. I will try debugging another way tonight or tomorrow.

Have a good evening.

@lgrignon
Copy link
Contributor Author

@markwongsk I found the problem and fix it, could you check it again please?

@tomshen
Copy link
Contributor

tomshen commented Sep 19, 2016

I tested the fix, and it seems like it works.

Unless @markwongsk has any further comments, I'll merge this in (and release) tomorrow night.

@tomshen tomshen merged commit 935dfaa into palantir:master Sep 20, 2016
@tomshen
Copy link
Contributor

tomshen commented Sep 20, 2016

Thanks for the contribution @lgrignon! Sorry it took so long to get in.

@Rouche
Copy link
Contributor

Rouche commented Dec 13, 2016

Is it live on the update site? Seems like i dont have the tsconfig option, and no updates availables.

Version i have: TypeScript 1.8.0.v20160504-1733 com.palantir.typescript.feature.feature.group Palantir Technologies, Inc.

Thanks.

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.

None yet

6 participants