Package scopes #4913

Closed
wants to merge 25 commits into
from

Conversation

Projects
None yet
8 participants
@weswigham
Member

weswigham commented Sep 21, 2015

After reading and commenting in #4665 and #4673, I felt like I'd gotten a pretty good idea for what we wanted to accomplish regarding package typings - and I believe our main concern was the pulling needs between properly representing the isolation that modules have from one another and the desire to enable consumer to reuse typings between "flat" or browser projects and commonjs based projects. I, personally, believed that a variety of package scoping was the correct solution if implementable and so looked into it and wrapped it up today - package scopes are very possible with our current compiler. The difficulty of adding a package scope was exaggerated.

While I haven't specified the exact behaviors of the package scope environment prior to my implementation, what I've changed internally is that any SourceFile can be a member of a package - a program puts a source file into a package if it find it via node module resolution and it was found transitively through either the node_modules folder or a package.json file and accompanying typings field. The entrypoint .d.ts file to that package (either index, a named .d.ts directly inside the node_modules folder, or a typings field reference) is considered its package "root" and uniquely identifies the package in the system. Please note - this is slightly different from the existing consideration for what an "external package" is for our errors - we neglected to consider finding a "package.json" outside a node_modules folder as a package (despite it clearly being a package since it has a package file) with the justification that it shouldn't be "external" in that case. Limiting the package-scoping feature to node_modules-located packages seems silly, though, so the definition of where a package begins was updated to include anything found via a package.json.

Continuing my explanation of the changes, after files are bound in the binder, normally file-locals for non-external-modules get merged into a single, global scope. With these changes, instead if a source file is a member of a package, those locals get merged into a package scope. Once the global environment has been setup and bound, the global scope is then merged into each package scope (I'll have to check out how this plays with interface merging and other global state). To pair with this newly created scope concept, at any point where previously the global scope may have been consulted, the package scope for the file containing that node is consulted first. Since the global scope is merged into the package scope, this implies that the global scope aught never to get meaningfully consulted if a package is present... but I don't enforce that right now, and I may have to create some scenarios to check for unexpected leakage.

TL;DR:
The end result? This is a valid configuration:

// @filename: node_modules/a/ref.d.ts
declare module "internal" {
    export var foo: void;
}

// @filename: node_modules/a/index.d.ts
/// <reference path="ref.d.ts"/>
export * from "internal";


// @filename: b.ts
import y = require("a");
import z = require("internal"); // should error, since "internal" isn't in this package

This lets people write their "isomorphic" typings (like ref.d.ts above) and then adapt them to a node package with a quick two-liner and without polluting the global scope. I think we can mostly agree that the above configuration can be a very desirable one for typescript users.

The only issue with this approach that I can readily think of is that stdlib expectation errors will still be reported - but that may actually be fine. This means I can target ES5, include something which relies on ES6 promises (and expects to use the es6 lib), and include es6-promise myself to polyfill it, or retarget against es6, depending on my needs.

@mhegazy @jbrantly @poelstra you all seemed invested/interested in improving the node module typing scenario - what are your opinions on this approach?

@jbrantly

This comment has been minimized.

Show comment
Hide comment
@jbrantly

jbrantly Sep 22, 2015

I think this is a huge step in the right direction! Thanks a ton for working on this. I haven't yet had a chance to actually try it out but some comments just based on your description.

Things that directly relate to this PR:

  1. Regarding "interface merging and other global state", I personally think that if something is defined in both the global scope and the package scope then the package scope should just "win". That way it's basically guaranteed that the package typings can "stand on their own". The application author can't somehow make the package typings fail by overriding some global. Maybe that's undesirable or I'm missing something, though.

  2. Does the current logic work recursively? For instance:

    • myapp
      • mylib
        • somedep
      • myotherlib

    In that scenario, does somedep get its own package scope so that there are essentially 4 scopes (global, mylib, somedep, myotherlib) or is it only first-level packages that get their own scope?

    Based on this:

    The entrypoint .d.ts file to that package (either index, a named .d.ts directly inside the node_modules folder, or a typings field reference) is considered its package "root" and uniquely identifies the package in the system

    It looks like the former which is good.

  3. The scope works for all globals, not just ambient external module declarations, right? eg.

    // @filename: node_modules/a/ref.d.ts
    declare namespace Internal {
      export var foo: void;
    }
    
    // @filename: node_modules/a/index.d.ts
    /// <reference path="ref.d.ts"/>
    export = Internal;

    ^ that would also work correctly and the symbol "Internal" would not be exposed.

Some items that might not directly pertain to this PR but are related:

  1. Have you given more thought on how to intentionally expose globals for node packages that do? With #4738 this currently seems impossible, but it's clearly a valid use-case (#4877). I understand if this is out-of-scope for now.
  2. Have you thought about CJS vs ES6 compatibility with the definitions (export default vs export =)? There is a discussion in #4673 about this. Again, probably out-of-scope for this PR but something that should probably be addressed in the final solution.

I think this is a huge step in the right direction! Thanks a ton for working on this. I haven't yet had a chance to actually try it out but some comments just based on your description.

Things that directly relate to this PR:

  1. Regarding "interface merging and other global state", I personally think that if something is defined in both the global scope and the package scope then the package scope should just "win". That way it's basically guaranteed that the package typings can "stand on their own". The application author can't somehow make the package typings fail by overriding some global. Maybe that's undesirable or I'm missing something, though.

  2. Does the current logic work recursively? For instance:

    • myapp
      • mylib
        • somedep
      • myotherlib

    In that scenario, does somedep get its own package scope so that there are essentially 4 scopes (global, mylib, somedep, myotherlib) or is it only first-level packages that get their own scope?

    Based on this:

    The entrypoint .d.ts file to that package (either index, a named .d.ts directly inside the node_modules folder, or a typings field reference) is considered its package "root" and uniquely identifies the package in the system

    It looks like the former which is good.

  3. The scope works for all globals, not just ambient external module declarations, right? eg.

    // @filename: node_modules/a/ref.d.ts
    declare namespace Internal {
      export var foo: void;
    }
    
    // @filename: node_modules/a/index.d.ts
    /// <reference path="ref.d.ts"/>
    export = Internal;

    ^ that would also work correctly and the symbol "Internal" would not be exposed.

Some items that might not directly pertain to this PR but are related:

  1. Have you given more thought on how to intentionally expose globals for node packages that do? With #4738 this currently seems impossible, but it's clearly a valid use-case (#4877). I understand if this is out-of-scope for now.
  2. Have you thought about CJS vs ES6 compatibility with the definitions (export default vs export =)? There is a discussion in #4673 about this. Again, probably out-of-scope for this PR but something that should probably be addressed in the final solution.
@weswigham

This comment has been minimized.

Show comment
Hide comment
@weswigham

weswigham Sep 22, 2015

Member
  1. Agreed. Though, again, I'm unsure how this should interact with interface merging. I've said before that it should do as it does now - shadow the global scope and merge internally to the package, but this still means that the top-level project author can introduce globals which appear/conflict with dependencies. This could be changed simply by giving the user project a package root and only adding the stdlib and builtins to the globals table. Also may need to change the operation used internally from merging symbol tables with the global one to copying them to prevent symbol merges from leaking between scopes. A symbol copy-on-write (merge) helper could be useful there... But yeah, agreed on the whole "user code shouldn't usually break packaged code" concept.
  2. Yes it should work recursively.
  3. Yes, it scopes all globals to the package, not just ambient module declarations.

And yes, it would be easier to deal with the other two issues with seperate proposals/PRs, IMO.

Member

weswigham commented Sep 22, 2015

  1. Agreed. Though, again, I'm unsure how this should interact with interface merging. I've said before that it should do as it does now - shadow the global scope and merge internally to the package, but this still means that the top-level project author can introduce globals which appear/conflict with dependencies. This could be changed simply by giving the user project a package root and only adding the stdlib and builtins to the globals table. Also may need to change the operation used internally from merging symbol tables with the global one to copying them to prevent symbol merges from leaking between scopes. A symbol copy-on-write (merge) helper could be useful there... But yeah, agreed on the whole "user code shouldn't usually break packaged code" concept.
  2. Yes it should work recursively.
  3. Yes, it scopes all globals to the package, not just ambient module declarations.

And yes, it would be easier to deal with the other two issues with seperate proposals/PRs, IMO.

@poelstra

This comment has been minimized.

Show comment
Hide comment
@poelstra

poelstra Sep 22, 2015

👍 Very nice!

I like the idea of being able to transition smoothly to 'proper external' modules (from current DT typings), by defining these two-liner files. It's basically my 'mixed-mode' idea in #4673, but then defined explicitly.

Still have to play with the actual code.

👍 Very nice!

I like the idea of being able to transition smoothly to 'proper external' modules (from current DT typings), by defining these two-liner files. It's basically my 'mixed-mode' idea in #4673, but then defined explicitly.

Still have to play with the actual code.

poelstra added a commit to basarat/typescript-node that referenced this pull request Sep 22, 2015

@poelstra

This comment has been minimized.

Show comment
Hide comment
@poelstra

poelstra Sep 22, 2015

Created a test repository: https://github.com/basarat/typescript-node/tree/master/poelstra4
See the README.md in that dir for instructions on reproducing the test.

Short conclusion: it works! 😃

  • Both versions of myutils can co-exist, i.e. the results from the function calls in myprogram.ts can be resolved,
    and the type of the .foo property is correct
  • The Promise type and e.g. Buffer are not available in myprogram.ts (this is indeed the intended behaviour!)
  • Typings for myutils need to be stored in node_modules to be found as external modules. They are typically
    provided by the 'parent' package (such as mylib and myotherlib), but the node_modules directory is not
    typically part of a repository/package. So either tooling should handle this, or a configurable search path
    (similar to, but different from typings) needs to be defined.
  • myotherlib contains a bluebird typing straight from DT, but also needs a 'wrapper' in
    myotherlib/node_modules/bluebird.d.ts to make myotherlib/index.d.ts work when included from myprogram.
    Same remark on having to provide the file in node_modules.

Any ideas on moving the typings out of node_modules?

Created a test repository: https://github.com/basarat/typescript-node/tree/master/poelstra4
See the README.md in that dir for instructions on reproducing the test.

Short conclusion: it works! 😃

  • Both versions of myutils can co-exist, i.e. the results from the function calls in myprogram.ts can be resolved,
    and the type of the .foo property is correct
  • The Promise type and e.g. Buffer are not available in myprogram.ts (this is indeed the intended behaviour!)
  • Typings for myutils need to be stored in node_modules to be found as external modules. They are typically
    provided by the 'parent' package (such as mylib and myotherlib), but the node_modules directory is not
    typically part of a repository/package. So either tooling should handle this, or a configurable search path
    (similar to, but different from typings) needs to be defined.
  • myotherlib contains a bluebird typing straight from DT, but also needs a 'wrapper' in
    myotherlib/node_modules/bluebird.d.ts to make myotherlib/index.d.ts work when included from myprogram.
    Same remark on having to provide the file in node_modules.

Any ideas on moving the typings out of node_modules?

@weswigham

This comment has been minimized.

Show comment
Hide comment
@weswigham

weswigham Sep 22, 2015

Member

Any ideas on moving the typings out of node_modules?

I always thought it was odd that the organization you have there works, but it's because our module resolution logic mimics node's, so a loose file is resolved before the real dependency in the node_modules folder. I hadn't actually thought about exploiting that logic to type your dependencies and get a package scope for each one, as you've done in your example. I always thought that if you needed to define types for your dependencies, you'd simply /// ref stuff in a typings folder, just like you would today. Since your definitions won't conflict with anyone including you, it's pretty safe.

Member

weswigham commented Sep 22, 2015

Any ideas on moving the typings out of node_modules?

I always thought it was odd that the organization you have there works, but it's because our module resolution logic mimics node's, so a loose file is resolved before the real dependency in the node_modules folder. I hadn't actually thought about exploiting that logic to type your dependencies and get a package scope for each one, as you've done in your example. I always thought that if you needed to define types for your dependencies, you'd simply /// ref stuff in a typings folder, just like you would today. Since your definitions won't conflict with anyone including you, it's pretty safe.

@poelstra

This comment has been minimized.

Show comment
Hide comment
@poelstra

poelstra Sep 23, 2015

I always thought that if you needed to define types for your dependencies, you'd simply /// ref stuff in a typings folder

My thought was (is) that we shouldn't need any /// ref lines when using external modules.

We no longer need them when modules provide their own typings, so it would be great if we also don't need them anymore for packages that don't provide their own.

In my examples, only node.d.ts remains, but this should IMO be handled more like lib.d.ts, and not provided by every single package. That way, even the tsd.d.ts that I referenced from tsconfig.json for now, can be omitted.

BTW: I'm wondering, would it be possible to apply your scoping concept to every single .ts file? (In case of commonjs, of course) That would very nicely solve the problem of globals leaking even between the .ts files in a package (e.g. that mocha's describe and it are available in files that aren't tests, and that jasmine and mocha can't be mixed in the same tsconfig context).

I always thought that if you needed to define types for your dependencies, you'd simply /// ref stuff in a typings folder

My thought was (is) that we shouldn't need any /// ref lines when using external modules.

We no longer need them when modules provide their own typings, so it would be great if we also don't need them anymore for packages that don't provide their own.

In my examples, only node.d.ts remains, but this should IMO be handled more like lib.d.ts, and not provided by every single package. That way, even the tsd.d.ts that I referenced from tsconfig.json for now, can be omitted.

BTW: I'm wondering, would it be possible to apply your scoping concept to every single .ts file? (In case of commonjs, of course) That would very nicely solve the problem of globals leaking even between the .ts files in a package (e.g. that mocha's describe and it are available in files that aren't tests, and that jasmine and mocha can't be mixed in the same tsconfig context).

@@ -180,7 +180,7 @@ module ts {
const actual = file.resolvedModules[id];
assert.isTrue(actual !== undefined);
assert.isTrue(expected.resolvedFileName === actual.resolvedFileName, `'resolvedFileName': expected '${expected.resolvedFileName}' to be equal to '${actual.resolvedFileName}'`);
- assert.isTrue(expected.isExternalLibraryImport === actual.isExternalLibraryImport, `'shouldBeProperExternalModule': expected '${expected.isExternalLibraryImport}' to be equal to '${actual.isExternalLibraryImport}'`);
+ assert.isTrue(expected.packageRoot === actual.packageRoot, `'shouldBeProperExternalModule': expected '${expected.packageRoot}' to be equal to '${actual.packageRoot}'`);

This comment has been minimized.

@DanielRosenwasser

DanielRosenwasser Sep 29, 2015

Member

Seems like these should use assert.equals

@DanielRosenwasser

DanielRosenwasser Sep 29, 2015

Member

Seems like these should use assert.equals

@alexeagle

This comment has been minimized.

Show comment
Hide comment
@alexeagle

alexeagle Oct 4, 2015

Contributor

Thanks for the pointer here, Wes. I see there was a lot of good discussion about this problem in early September. Are you and the team still actively working on it?

Contributor

alexeagle commented Oct 4, 2015

Thanks for the pointer here, Wes. I see there was a lot of good discussion about this problem in early September. Are you and the team still actively working on it?

src/compiler/types.ts
@@ -1233,6 +1233,10 @@ namespace ts {
isBracketed: boolean;
}
+ export interface PackageDescriptor extends Scope {
+ packageFile: string;

This comment has been minimized.

@DanielRosenwasser

DanielRosenwasser Nov 3, 2015

Member

Can you add a comment? Specifically mention that this can be a package.json or index.d.ts.

@DanielRosenwasser

DanielRosenwasser Nov 3, 2015

Member

Can you add a comment? Specifically mention that this can be a package.json or index.d.ts.

src/compiler/program.ts
- result = loadNodeModuleFromDirectory(candidate, /* loadOnlyDts */ true, failedLookupLocations, host);
- if (result) {
- return { resolvedModule: { resolvedFileName: result, isExternalLibraryImport: true }, failedLookupLocations };
+ let res = loadNodeModuleFromDirectory(candidate, /* loadOnlyDts */ true, failedLookupLocations, host, /*mustBePackage*/ true);

This comment has been minimized.

src/compiler/program.ts
@@ -116,7 +114,10 @@ namespace ts {
failedLookupLocation.push(packageJsonPath);
}
- return loadNodeModuleFromFile(combinePaths(candidate, "index"), loadOnlyDts, failedLookupLocation, host);
+ let result = loadNodeModuleFromFile(combinePaths(candidate, "index"), loadOnlyDts, failedLookupLocation, host);

This comment has been minimized.

src/compiler/checker.ts
@@ -14881,6 +14914,10 @@ namespace ts {
}
anyArrayType = createArrayType(anyType);
+
+ forEachValue(packages, package => { // Once all packages are merged and global scope is setup, merge global scope into each package

This comment has been minimized.

@DanielRosenwasser

DanielRosenwasser Nov 3, 2015

Member

Move comment above

@DanielRosenwasser

DanielRosenwasser Nov 3, 2015

Member

Move comment above

src/compiler/checker.ts
+ if (!packages[file.package.packageFile]) {
+ packages[file.package.packageFile] = file.package;
+ }
+ file.package = packages[file.package.packageFile]; // Dedupe packages

This comment has been minimized.

@DanielRosenwasser

DanielRosenwasser Nov 3, 2015

Member

This should be in an else, and move the comment to above the line.

@DanielRosenwasser

DanielRosenwasser Nov 3, 2015

Member

This should be in an else, and move the comment to above the line.

src/compiler/checker.ts
@@ -14823,10 +14845,21 @@ namespace ts {
bindSourceFile(file);
});
- // Initialize global symbol table
+ let packages: Map<PackageDescriptor> = {};

This comment has been minimized.

src/compiler/checker.ts
@@ -967,7 +975,8 @@ namespace ts {
}
let isRelative = isExternalModuleNameRelative(moduleName);
if (!isRelative) {
- let symbol = getSymbol(globals, "\"" + moduleName + "\"", SymbolFlags.ValueModule);
+ let file = getSourceFileOfNode(location);
+ let symbol = getSymbol(file.package ? file.package.symbols : globalScope.symbols, "\"" + moduleName + "\"", SymbolFlags.ValueModule);

This comment has been minimized.

src/compiler/checker.ts
@@ -125,7 +125,9 @@ namespace ts {
let anySignature = createSignature(undefined, undefined, emptyArray, anyType, undefined, 0, false, false);
let unknownSignature = createSignature(undefined, undefined, emptyArray, unknownType, undefined, 0, false, false);
- let globals: SymbolTable = {};
+ let globalScope: Scope = {

This comment has been minimized.

@DanielRosenwasser

DanielRosenwasser Nov 3, 2015

Member

I get why it's nice to have this, but you don't actually end up using globalScope on its own - we always just grab symbols anyhow.

@DanielRosenwasser

DanielRosenwasser Nov 3, 2015

Member

I get why it's nice to have this, but you don't actually end up using globalScope on its own - we always just grab symbols anyhow.

@weswigham

This comment has been minimized.

Show comment
Hide comment
@weswigham

weswigham Nov 5, 2015

Member

New work items for this PR:

  • Merge with master
  • Collapse imports of the form package/entrypoint and package/second into a single package scope. The current behavior for this case is ambiguous.
  • Throw an error when a file tries to be included in multiple package scopes (for example, if multiple packages reach outside of themselves and directly triple-slash-reference a file in a third package.) The current behavior for this case is ambiguous and the usage is uncommon. (Plus, the error can be relaxed later if need be)
  • Allow an index.d.ts which is ambient - when it is, pull out an ambient declaration whose name matches the package name from the package scope and use its exports as that package's exports.
Member

weswigham commented Nov 5, 2015

New work items for this PR:

  • Merge with master
  • Collapse imports of the form package/entrypoint and package/second into a single package scope. The current behavior for this case is ambiguous.
  • Throw an error when a file tries to be included in multiple package scopes (for example, if multiple packages reach outside of themselves and directly triple-slash-reference a file in a third package.) The current behavior for this case is ambiguous and the usage is uncommon. (Plus, the error can be relaxed later if need be)
  • Allow an index.d.ts which is ambient - when it is, pull out an ambient declaration whose name matches the package name from the package scope and use its exports as that package's exports.
@basarat

This comment has been minimized.

Show comment
Hide comment
@basarat

basarat Nov 5, 2015

Contributor

/cc @blakeembrey although I am pretty sure he knows 🌹

Contributor

basarat commented Nov 5, 2015

/cc @blakeembrey although I am pretty sure he knows 🌹

@weswigham

This comment has been minimized.

Show comment
Hide comment
@weswigham

weswigham Nov 14, 2015

Member

@mhegazy This PR has been synced with master and had the design changes we discussed (listed above) integrated. Do we want to keep looking at it?

Member

weswigham commented Nov 14, 2015

@mhegazy This PR has been synced with master and had the design changes we discussed (listed above) integrated. Do we want to keep looking at it?

nozzlegear added a commit to nozzlegear/Shopify-Prime that referenced this pull request Jun 4, 2016

Remove Node definition, which pollutes global namespace and prevents …
…dependent TS projects from building

When a package has Shopify-Prime as a dependency, and has its own node.d.ts definition installed, the two definitions will clash and break the dependent's build because globals aren't scoped to their own packages. There is some work being done by MS at (Microsoft/TypeScript#4913) to fix the problem.
@mhegazy

This comment has been minimized.

Show comment
Hide comment
@mhegazy

mhegazy Jul 21, 2016

Contributor

This does not seem to be needed now with the move to @types. closing.

Contributor

mhegazy commented Jul 21, 2016

This does not seem to be needed now with the move to @types. closing.

@mhegazy mhegazy closed this Jul 21, 2016

@weswigham

This comment has been minimized.

Show comment
Hide comment
@weswigham

weswigham Jul 21, 2016

Member

@mhegazy Yep, @types and UMD definitions solve this issue pretty nicely.

Member

weswigham commented Jul 21, 2016

@mhegazy Yep, @types and UMD definitions solve this issue pretty nicely.

@weswigham weswigham deleted the weswigham:package-scopes branch Aug 22, 2017

@Microsoft Microsoft locked and limited conversation to collaborators Jun 19, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.