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

services tsconfig.json added missing file references and fixed the order #2917

Merged
merged 1 commit into from
Apr 27, 2015
Merged

Conversation

basarat
Copy link
Contributor

@basarat basarat commented Apr 25, 2015

This order is significant:

        "../compiler/diagnosticInformationMap.generated.ts",
        "../compiler/commandLineParser.ts",

Also not having these extra files referenced caused TypeStrong/atom-typescript#304

Really wish we moved to external modules, but that's a seperate issue 🌹

This order is significant:

```ts
        "../compiler/diagnosticInformationMap.generated.ts",
        "../compiler/commandLineParser.ts",
```

Also not having these extra files referenced caused TypeStrong/atom-typescript#304 
Really wish we moved to external modules.
@msftclas
Copy link

Hi @basarat, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@DanielRosenwasser
Copy link
Member

Actually I think that in general diagnosticInformationMap.generated.ts should go before anything that actually reports diagnostics (i.e. scanner.ts as well).

Also maybe declarationEmitter.ts should also go before emitter.ts as well. Not that these matter in practice, but it's better to order these to make the dependencies more explicit.

@basarat
Copy link
Contributor Author

basarat commented Apr 25, 2015

Not that these matter in practice

True in general but in this case they do matter. Consider the code staight out of repo : types,commandLineParser,diagnostic:

// Types.ts
module ts {
    export enum DiagnosticCategory {
        Warning,
        Error,
        Message,
    }
}

// commandLineParser.ts
module ts {
    /* @internal */
    export var optionDeclarations = [
        {
            name: "declaration",
            shortName: "d",
            type: "boolean",
            description: Diagnostics.Generates_corresponding_d_ts_file,
        }
    ]
}

// diagnostic generated
module ts {
    export var Diagnostics = {
        Generates_corresponding_d_ts_file: { code: 6002, category: DiagnosticCategory.Message, key: "Generates corresponding '.d.ts' file." },
    }
}

If you run this on the playground it will fail with Uncaught TypeError: Cannot read property 'Generates_corresponding_d_ts_file' of undefined . Fix is to reorder the diagnostics to come before commandLineParser, hence this PR.

It matters if the stuff is used during initial module body evaluation / execution (e.g. the var optionDeclarations).

This is exactly why I have come to hate out : https://github.com/TypeStrong/atom-typescript/blob/master/docs/out.md 🌹

mhegazy added a commit that referenced this pull request Apr 27, 2015
services tsconfig.json added missing file references and fixed the order
@mhegazy mhegazy merged commit 2349a27 into microsoft:master Apr 27, 2015
@mhegazy
Copy link
Contributor

mhegazy commented Apr 27, 2015

thanks!

@basarat basarat deleted the patch-1 branch April 27, 2015 23:38
@basarat
Copy link
Contributor Author

basarat commented Apr 27, 2015

🌹

@basarat basarat mentioned this pull request Apr 28, 2015
@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants