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

rfc: import syntax #3352

Open
Chriscbr opened this issue Jul 10, 2023 · 18 comments
Open

rfc: import syntax #3352

Chriscbr opened this issue Jul 10, 2023 · 18 comments
Labels
✨ enhancement New feature or request 📐 language-design Language architecture 📚 libraries Wing Libraries needs-discussion Further discussion is needed prior to impl

Comments

@Chriscbr
Copy link
Contributor

Chriscbr commented Jul 10, 2023

Use cases

  • Export types from a Wing module
  • Import all types from a Wing module, so they can accessed under a single namespace
  • Import all types from a Wing module, merging with the current namespace
  • Import a few types from another Wing module, so they are all in the current namespace
  • Import types from another Wing module, renaming some of them to avoid name collisions
  • Import constant values from another Wing files
  • Disallow importing private types/constants
  • Organize types into hierarchies of modules (like we have in Wing's standard library), and support making those modules public or private
  • Re-export types from another Wing file

Design goals:

  • Syntax should be readable and easy to understand
  • Syntax should have granularity for when you only want to import a few types from a file, or when you want to rename an import
  • Syntax should let you go fast when you want to (i.e. a simple syntax to import everything)
  • Syntax should be amenable to IDE autocompletions (thanks @MarkMcCulloh)
  • Syntax should be amenable to refactoring with the LSP
  • Syntax should make it easy to understand where a type is coming from

In the set of examples that follows, lib.w is a Wing module / JSII type system that contains roughly:

pub module foo {
  pub class Foo {}
  
  pub module inner {
    pub class InnerFoo {}
    pub class InnerBar {}
  }
}

pub class Bar {}

Use case 1: Import a few types/submodules from a module, giving them new names

Approach 1:

bring "./lib.w" {
    foo as my_foo,
    Bar as MyBar
};

new my_foo.Foo();
new MyBar();

Approach 2:

bring "./lib.w" as my_lib;

let my_foo = my_lib.foo;
let MyBar = my_lib.Bar;

new my_foo.Foo();
new MyBar();

Approach 3:

bring "./lib.w".foo as my_foo;
bring "./lib.w".Bar as MyBar;

new my_foo.Foo();
new MyBar();

Use case 2: Import an entire module, assigning it a name

Approach 1:

bring "./lib.w" as my_lib;

Approach 2:

bring "./lib.w" as my_lib;

Approach 3:

bring "./lib.w" as my_lib;

(no difference)

Use case 3: Deep-import types from a module

Approach 1:

bring "./lib.w" {
    foo.inner.InnerFoo as MyInnerFoo,
    foo.inner.InnerBar,
};

new MyInnerFoo();
new InnerBar();

Approach 2:

bring "./lib.w" as my_lib;

let MyInnerFoo = my_lib.foo.inner.InnerFoo;
let InnerBar = my_lib.inner.InnerBar;

new MyInnerFoo();
new InnerBar();

Approach 3:

bring "./lib.w".foo.inner.Foo as MyInnerFoo;
bring "./lib.w".foo.inner.InnerBar;

new MyInnerFoo();
new InnerBar();

Use case 4: Re-export imported modules or types

Approach 0:

Avoid adding a re-export syntax, and instead favor explicitly defining modules using pub module syntax and type aliases.

Approach 1:

// file1.w
pub bring "./lib.w" {
    foo as my_foo,
    Bar as MyBar
};

// file2.w
bring "./file1.w" as file1;

new file1.my_foo.Foo();
new file1.MyBar();

Approach 2:

// file1.w
bring "./lib.w" as my_lib;

pub let my_foo = my_lib.foo;
pub let MyBar = my_lib.Bar;

// file2.w
bring "./file1.w" as file1;

new file1.my_foo.Foo();
new file1.MyBar();

Approach 3:

N/A

@Chriscbr Chriscbr added ✨ enhancement New feature or request 📐 language-design Language architecture labels Jul 10, 2023
@MarkMcCulloh
Copy link
Contributor

MarkMcCulloh commented Jul 10, 2023

We should find a way to designate a module first before selecting specific parts of it to import. e.g. instead of:

bring Baz from "./file2.w"; // import only Baz

we should have something like:

bring "./file2.w" with Baz
// or
bring "./file2.w" with { Baz }
// or (with aliasing)
bring "./file2.w" with { Baz as Foo }

This allows the language server to offer real suggestions when typing bring "./file2.w" with . It also provides some nice homogeneity to the start of each bring statement

@marciocadev
Copy link
Collaborator

marciocadev commented Jul 11, 2023

// file1.w
bring cloud;

pub class Foo {
  store: cloud.Bucket;
  pub key: str;
  init() {
    this.store = new cloud.Bucket();
    this.key = "data.json";
  }
}

class Bar {}

pub class UsesBar {
  pub bar: Bar; // ERROR: private type `Bar` in public field
  init() {
    this.bar = new Bar();
  }
}

In the code above, I found the error strange since, despite the Bar class being private, it was being used as the type of the public variable bar in the UsesBar class.

To me, the pub prefix would indicate that the variable becomes public, without being tied to the possibility of being public or not by the type assigned to the variable.

I find it counterintuitive for the visibility of a variable to be defined by the type assigned to it.

@marciocadev
Copy link
Collaborator

We should find a way to designate a module first before selecting specific parts of it to import. e.g. instead of:

bring Baz from "./file2.w"; // import only Baz

we should have something like:

bring "./file2.w" with Baz
// or
bring "./file2.w" with { Baz }
// or (with aliasing)
bring "./file2.w" with { Baz as Foo }

This allows the language server to offer real suggestions when typing bring "./file2.w" with . It also provides some nice homogeneity to the start of each bring statement

I liked that suggestion. Whenever I do an import in TypeScript, I usually write:

import {} from "lib";

and then go back to the {} to use auto-complete.

If the library is defined first, it's better for using auto-complete.

@MarkMcCulloh
Copy link
Contributor

Import constant values from another Wing files

What should the export syntax look like? I don't think it's mentioned in the issue.
I'm assuming:

pub let COOL = 2;

If so, does constant here just mean no var, or interior mutability as well? e.g. is this allowed?

pub let COOL_STUFF = MutJson {};

@hasanaburayyan
Copy link
Contributor

hasanaburayyan commented Jul 11, 2023

liked that suggestion. Whenever I do an import in TypeScript, I usually write:
import {} from "lib";
and then go back to the {} to use auto-complete.
If the library is defined first, it's better for using auto-complete

+1 on having {} at the end. Though something doesnt feel right to me with bring "./file2.w" with { Baz } Saying bring x with {y, z} sounds like y and z are in addition to x or it mutates x in someway. When really they are subset of the set x

maybe just bring from "./file2.w" { Baz }?

trying to think of what is the most intuitive.

@WeepingClown13
Copy link
Collaborator

liked that suggestion. Whenever I do an import in TypeScript, I usually write:
import {} from "lib";
and then go back to the {} to use auto-complete.
If the library is defined first, it's better for using auto-complete

+1 on having {} at the end. Though something doesnt feel right to me with bring "./file2.w" with { Baz } Saying bring x with {y, z} sounds like y and z are in addition to x. When really they are subset of the set x

maybe just bring from "./file2.w" { Baz }"?

trying to think of what is the most intuitive.

This is more or less what I felt. Having with feels we are bringing the module and then some extra parts. Though I can't exactly think of a very intuitive parallel. +1 to having the module first and components last.

@marciocadev
Copy link
Collaborator

liked that suggestion. Whenever I do an import in TypeScript, I usually write:
import {} from "lib";
and then go back to the {} to use auto-complete.
If the library is defined first, it's better for using auto-complete

+1 on having {} at the end. Though something doesnt feel right to me with bring "./file2.w" with { Baz } Saying bring x with {y, z} sounds like y and z are in addition to x or it mutates x in someway. When really they are subset of the set x

maybe just bring from "./file2.w" { Baz }?

trying to think of what is the most intuitive.

why not use something like from "./file2.w" bring Baz

Feels like python

@Chriscbr
Copy link
Contributor Author

Chriscbr commented Jul 11, 2023

We had an earlier discussion about imports (video). It seems like we've converged on at least two directions for import syntaxes.

Direction 1: Use the bring keyword to import modules, submodules, and individual types.

Direction 2: Use the bring keyword to import modules, and modules only.
bring is not used to import submodules or to import individual types.
To import submodules and import individual types, use the alias keyword (could also be let type or typealias etc.)

Ultimately we might be able to support both (as @MarkMcCulloh suggested) but I thought I'd try at least to steelman both sides to get a feel for both sides, and get folks' opinions on it.

EDIT: the options have now been added to the first post as "Approach 1" and "Approach 2"

@MarkMcCulloh
Copy link
Contributor

I think a deep import syntax shouldn't force you to re-alias types if it's not needed. e.g. for a deep import I think it would be interesting to allow this:

bring "./lib.w" {
    foo.inner.MyInnerFoo,
};

new MyInnerFoo();

Basically a combination of a using-esque statement and a traditional import

@ainvoner
Copy link
Contributor

ainvoner commented Jul 13, 2023

Console point of view:

Currently to support multiple files (external files) the console is watching the main file parent directory excluding node_modules and target directories.

A better solution would be to get the list of files to watch from the compiler.
Currently compile(entrypoint: string, options: CompileOptions) return value is the synthDir. An additional array of files paths to watch would be better.

@Chriscbr
Copy link
Contributor Author

@ainvoner Makes sense to me. 👍 Off the top of my head it sounds like information the compiler should be able to produce. (Maybe it would also be useful for the LSP? @MarkMcCulloh)

@Chriscbr
Copy link
Contributor Author

@ainvoner Is it possible you can use a simple heuristic like simply watching all .w files inside of the users' current directory (whichever directory wing it was run in, or that the VS Code has opened)? A more fine grained API sounds better, I'm just curious what kinds of issues we start running into when doing the naive thing.

@ainvoner
Copy link
Contributor

@Chriscbr we allow users to load external files into their .w files so watching only .w files under the parent directory is not enough (external files can be changed / added during development).

Currently, in order to support the above use case we watch all files in the .w parent directory and exclude node_modules and target directories.

In a real life use case (even in our small dogfooding project for winglang.io reverse proxy), watching the parent directory of the main.w file will result watching unnecessary files that shouldn't trigger a new compile process of our application (.github, README.md etc...)

@Chriscbr
Copy link
Contributor Author

Chriscbr commented Jul 26, 2023

@ainvoner Got it. If I understand you correctly this means the list of files to watch needs to include .w files as well as external JS files (referenced from extern methods).

I don't think there is a fool-proof approach with our current language design since it's possible the extern JS file will reference other JS files on the file system, or npm modules, etc.

Example 1:

// hello.w
class Util {
  extern "./helpers.js" inflight foo();
}

// helpers.js
exports.foo = function() {
  require("./other.js").bar();
}

// other.js
exports.bar = function() {
  console.log("hello");
}

Currently the compiler doesn't analyze the source code of extern files (like helpers.js). It only hands the path of the file to the SDK, which uses esbuild to bundle all JS files into a inflight code bundles. The result: if you modify other.js, the Wing Console will not automatically reload.

Example 2:

// hello.w
class Util {
  extern "./helpers.js" inflight foo();
}

// helpers.js
exports.foo = function() {
  const { minimatch } = require('minimatch')
  minimatch('bar.foo', '*.foo');
}

In this example, if the version of minimatch changes because it's reinstalled (or updated to a different version), then the Wing Console would need to be reloaded to reflect the changes.

I think it still makes sense for the compiler to give a "best-effort" attempt at providing a list of files to watch. But my guess is that we will also need a reload/refresh button in the Wing Console.

@Chriscbr
Copy link
Contributor Author

Our team had another discussion about the merits of the syntax options. We didn't finalize any syntax decisions, but a third new syntax proposal was brought to the table (I've added it as "Approach 3" in the first issue), and we had a few takeaways:

  1. "everything"-imports (like Python's import * from foo) is not a capability we want to support since it can introduce name collisions, and it means upstream modules can cause breaking changes by declaring new types
  2. bring statements can likely be limited to use at the top of the modules
  3. Type aliases would be nice to have, independent of their use for sub-importing types / having type aliases doesn't necessarily detract from the usefulness of importing or renaming types in bring statements.

@WeepingClown13
Copy link
Collaborator

I like that avoiding import * part. Even with python I try to avoid that as much as possible even if it gets a bit verbose, unless when I really have to import literally everything from a quite big module. God knows what all error it could bring forth. Won't even know what we are missing at a first glance because of the * instead of the method names.

@staycoolcall911 staycoolcall911 added the needs-discussion Further discussion is needed prior to impl label Oct 1, 2023
@Chriscbr Chriscbr removed their assignment Oct 13, 2023
@staycoolcall911 staycoolcall911 added the 📚 libraries Wing Libraries label Nov 15, 2023
Copy link

Hi,

This issue hasn't seen activity in 90 days. Therefore, we are marking this issue as stale for now. It will be closed after 7 days.
Feel free to re-open this issue when there's an update or relevant information to be added.
Thanks!

Copy link

Hi,

This issue hasn't seen activity in 90 days. Therefore, we are marking this issue as stale for now. It will be closed after 7 days.
Feel free to re-open this issue when there's an update or relevant information to be added.
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ enhancement New feature or request 📐 language-design Language architecture 📚 libraries Wing Libraries needs-discussion Further discussion is needed prior to impl
Projects
Status: 🤝 Backlog - handoff to owners
Development

No branches or pull requests

7 participants