Skip to content

Support export * #249

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

Closed
wants to merge 4 commits into from
Closed

Conversation

andy-hanson
Copy link
Contributor

@andy-hanson andy-hanson commented Sep 8, 2018

Fixes #27 (import * is working already).

Implementation notes:

  • FIrst we resolve all queued imports, then all queued exports, then all export *. But in every one of these phases we call into the same function resolveAndSetExport, so they share most of their code except for error messages.
  • It's an error to have two export * exporting the same name, but not an error to shadow an export * with a named export. So when resolving exports we fill in an exportsFromExportStar map to allow us to detect which exports came from export *.
  • In the final phase of resolving every export *, we start from the map from source -> sources it re-exports, from to get a map from source -> sources that re-export it. Then at each export, we spread it outwards as far as possible. If we did the reverse and tried to "pull" exports into a re-exporter, we run into problems when modules circularly re-export each other.
  • Instead of looking up imports in both index and non-index location we eagerly remove /index from source file paths. This will lead to conflicts if both foo.ts and foo/index.ts exist, although that already would have caused problems before, since imports would be resolved successfully from both files (where only one should work). Perhaps it should just be an error to have both foo.ts and foo/index.ts.

@torch2424
Copy link
Contributor

Thank you sooooo much for doing this!!!!! 😄

@MaxGraey
Copy link
Member

MaxGraey commented Sep 8, 2018

Great!

src/ast.ts Outdated

// "foo/index" -> "foo"
// "foo/bar" -> itself
export function stripIndex(internalPath: string): string {
Copy link
Member

Choose a reason for hiding this comment

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

Before, this worked as follows: On import "some/file", the compiler first requested some/file.ts from the frontend (here: asc), and if that didn't exist, some/file/index.ts. Both had a distinct internal name (either some/file or some/file/index), and their declarations were not mixed in any way. From a source file, one could both import some/file and some/file/index, if both would happen to exist. This matches what node.js is doing, and I wonder why a change like stripping /index is necessary, effectively removing that functionality?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated this branch so we can support foo.ts and foo/index.ts simultaneously.

There actually was a bug due to being too permissive and trying to resolve imports from both foo.ts and foo/index.ts:

foo.ts: export const foo1 = 0;
foo/index.ts: export const foo2 = 0;
user.ts:

import {} from "./foo"; //ensure file loaded
import { foo1 } from "./foo/index"; // Shouldn't compile

export function test(): i32 {
  return foo1;
}

Shouldn't compile, but did. Fixed in this branch. (Would be nice if we could have negative tests to verify that this isn't allowed, is there an issue open for that?)

@@ -77,6 +77,7 @@
"A parameter property cannot be declared using a rest parameter.": 1317,

"Duplicate identifier '{0}'.": 2300,
"Identifier '{0}' is re-exported from modules '{1}' and '{2}'.": 2301,
Copy link
Member

Choose a reason for hiding this comment

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

The convention for new errors that do not exist in TS I used so far is to use a 3-digit code instead (TS has 4 digit codes only), in this case that'd be the next 2XX one.

For reference, the original TS error here is

    "Initializer of instance member variable '{0}' cannot reference identifier '{1}' declared in the constructor.": {
        "category": "Error",
        "code": 2301
},

Copy link
Contributor Author

@andy-hanson andy-hanson Sep 9, 2018

Choose a reason for hiding this comment

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

Looks like TS already has a diagnostic for this situation, so I'll use that instead of creating an AS-specific one.

@@ -0,0 +1,22 @@
export class MultiMap<K, V> {
Copy link
Member

Choose a reason for hiding this comment

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

This creates a new GC object around a GCed map. It seems that this could be avoided by extending Map<K,V> instead, which should work in AS, but might not "just work" in TS. Have you tried that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately that won't work with --target es5. I can just add a multiMapAdd helper function though.

}
} else {
this.map.set(key, [value]);
export function multiMapAdd<K, V>(map: Map<K, V[]>, key: K, value: V) : void {
Copy link
Member

Choose a reason for hiding this comment

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

What do you think of naming the file src/util/map.ts so it can be used for all sorts of map helpers in the future, and maybe the function addOne?

@MaxGraey
Copy link
Member

cc @dcodeIO wdyt? Is it ready for merging?

@dcodeIO dcodeIO mentioned this pull request Feb 20, 2019
5 tasks
@dcodeIO dcodeIO closed this in #489 Feb 20, 2019
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.

Implement "export/import * from"
4 participants