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

Alternative syntax: Just import declaration in block #5

Open
Jamesernator opened this issue Jan 29, 2021 · 3 comments
Open

Alternative syntax: Just import declaration in block #5

Jamesernator opened this issue Jan 29, 2021 · 3 comments

Comments

@Jamesernator
Copy link

Jamesernator commented Jan 29, 2021

As the title says, a previous proposal with similar goals simply had import declarations within blocks. This feels pretty natural to me as it makes it very clear as to where evaluation happens (at the start of the block containing the import).

e.g.:

export function baz() {
  import foo from "bar"; // Evaluation/errors are deferred until here
}

export function buzz() {
  // failures in baz() don't affect this function
}

It also has the nice property of making it clear that some APIs might have specific boundaries for example in this API we can throw an error if inspect code is called from a non-node environment that doesn't support util, but still avoid making the API unncessarily async.:

export default class Foo {
  [Symbol.for('nodejs.util.inspect.custom')]() {
    import util from "util";
    return `Foo { ${ foo.data } }`;
  }
}
@codehag
Copy link
Collaborator

codehag commented Jan 30, 2021

Thanks for raising this! The alternative syntax is really for a slightly different proposal: conditional sync imports, but this use case was also on my mind when I was working through this proposal. For reference, the syntax you are proposing has already been considered by committee and was evolved into dynamic import. There are a couple of consequences as a result.

One way to look at it is "this restricts our design space", but there was a reason they eventually settled on it being a function, and I think it is fortunate actually in terms of expressiveness! A user can apply their understanding of functions to the import. But, we can also achieve your desired behaviour because of the relationship between static and dynamic imports. I will get to that in a moment (it is at the bottom).

I first want to show why the behaviour, which works well for the use case you’ve outlined (conditional sync import), isn't working quite as well as for the one in the readme (eval on first use), though its passable. (You know the saying, dress for the job you want -- that but for functionality)

We can take a look at the following example. Let’s say we have the following:

import { aMethod, someConst } from "./my-module";

function rarelyUsedA() {
  // ...
  aMethod();
}

function alsoRarelyUsedA() {
  // ...
  use(someConst);
}

We want to do some performance tuning. With the current proposal it becomes

import { aMethod, someConst } from "./my-module" with { lazyInit: true }

function rarelyUsedA() {
  // ...
  aMethod();
}

function alsoRarelyUsedA() {
  // ...
  use(someConst);
}

However, with the behaviour (and syntax) you are proposing it becomes



// showing a "simplified" version.
function lazyAMethod(...args) {
   import { aMethod } from "./my-module";
   return aMethod(...args);
}

// this is closer to what we need to do to emulate the behaviour that is proposed in this repo with the suggested new behaviour
Object.defineProperty(globalThis, "someConst", {
  get: function() {
    delete globalThis.someConst;
    import { someConst as value } from "./my-module"; // highlighting this, as it is problematic for code reuse, covered later
    Object.defineProperty(globalThis, "someConst", {
      value,
      writable: true,
      configurable: true,
      enumerable: true,
    });
    return value;
  },
  configurable: true,
  enumerable: true,
});


function rarelyUsedA() {
  // ...
  lazyAMethod();
}

function alsoRarelyUsedA() {
  // ...
  use(someConst);
}

It is a little worse for the use case I am trying to solve since we will still need to use a redefine getter (here is one painful example where importing multiple times makes the code less clear. We want to do performance tuning, not communicate conditional import -- import is unconditional. Many of the lazy getter related imports are what I am concerned about ). It boils down to the fact that this is really a different proposal as it has a different goal: conditional sync imports, but we could build what I want out of it. I wouldn't be surprised if that happens but I would prefer semantics that directly solve this problem.

Regarding the syntax specifically -- I am quite certain that we cannot use what you suggested. There is the collision with dynamic import to consider. It isn’t “self evident” wrt how dynamic import and this are different just from the syntax, and it conflates dynamic imports with static ones (prefetch or no). This would negatively impact learnability since we would have a conflation between dynamic is static with no clear distinction between the two. Additionally, with regards to building the feature proposed in this repo, it would not be possible with the syntax -- you cannot programatically bind module names, so for example the object getter above would not be possible, and would have to either be done manually or generated (which would be bad).

That doesn’t meant that there isn’t a use case for conditional import that is sync -- only the syntax proposed will not work. In fact, as mentioned i was already thinking about It when I suggested the syntax for my use case — it overlaps with dynamic import quite well because it reuses import attributes, which are valid in dynamic import. I haven’t decided on the word yet, in the readme I use lazyInit but as an alternative let’s consider prefetch and look at your example. Check this out:

export default class Foo {
  [Symbol.for('nodejs.util.inspect.custom')]() {
    const util = import(“util”, { with: { prefetch: true });
    return `Foo { ${ foo.data } }`;
  }
}

This would allow you to do a sync eval and get the behaviour that you want.

Ideally we would find a word between lazyInit and prefetch that communicates both cases well. Let's try prefetch for illustrative purposes.

import { foo, someConst } from "./bar" with { prefetch: true };

function baz() {
  const util = import("./util,js", { with: { prefetch: true }); // eval util 
  foo(utils); // potential first use  -- if yes, eval bar
}

function bazAnother() {
  const another = import("./another,js", { with: { prefetch: true }); // eval another
  another(someConst); // potential first use -- if yes, eval bar
}

This is part of the intention for using import attributes and assertions: it would potentially transfer to dynamic import. The benefits here is we don't have a conflation between static and dynamic imports, we use the same patterns as before. We just build on top of existing building blocks. There may be some difficulty getting this through because this would be prefetched dynamic import, but I think it is spec-able (pending some specification gymnastics).

Does prefetch as the attribute name communicate the shared behaviour well? I am not sure yet, and it feels a bit worse for communicating what the static import is doing. In some ways it addresses the concerns around the side-effectfulness of module import evaluation. Maybe we should have this attribute only for dynamic import... (well, that would suck, but hey, then this proposal would be what you are asking for effectively, with a bit more boiler plate for the use cases I am looking at).

Anyway. I am open to suggestions about what word this should be and would be very happy if this worked in both cases. I am looking for the right word that would cover the requirements raised by others as well. For example, I considering was lazyGetter to address the side effect issue. There are several open questions still. Alternatively these two can be split because they are doing slightly different things.

What do you think?

@Jamesernator
Copy link
Author

Jamesernator commented Feb 13, 2021

However, with the behaviour (and syntax) you are proposing it becomes

Actually I was just suggesting that in your example it should be rewritten as:

function rarelyUsedA() {
  import { aMethod } from "./my-module";
  // ...
  aMethod();
}

function alsoRarelyUsedA() {
  import { someConst } from "./my-module";
  // ...
  use(someConst);
}

The conditional imports was really just a bonus, as the syntax would naturally explain it. The reason for that is that if evaluation errors in ./my-module are already thrown at use-site, then so can linking errors be thrown at use-site.

The main reason I suggested the nested syntax is actually more so because the current proposal as specified basically turns declared variables into getters. This is as much a potential footgun as it is a benefit. In particular the footgun arises with errors and really slow evals, there is quite a risk of change in code could mean that evaluation happens at a different reference site to what is expected (or some might just be overlooked entirely). At least with multiple declarations a programmer is more likely to consider each location individually.

It is a little worse for the use case I am trying to solve since we will still need to use a redefine getter (here is one painful example where importing multiple times makes the code less clear.

I personally don't agree it makes the code less clear. In fact it's less clear to me what the performance/failure characteristics of any given method are now, as if eval is deferred failures or performance might happen late. I agree it makes the code longer, but personally I think conciseness is not worth unpredictability.

Regarding the syntax specifically -- I am quite certain that we cannot use what you suggested. There is the collision with dynamic import to consider. It isn’t “self evident” wrt how dynamic import and this are different just from the syntax, and it conflates dynamic imports with static ones (prefetch or no). This would negatively impact learnability since we would have a conflation between dynamic is static with no clear distinction between the two. Additionally, with regards to building the feature proposed in this repo, it would not be possible with the syntax -- you cannot programatically bind module names, so for example the object getter above would not be possible, and would have to either be done manually or generated (which would be bad).

Yeah, I think you're right that this would likely to be misused when dynamic import would be more appropriate. Though I would still strongly rather see a syntax that forces a developer to consider all the locations that use the imported variables, maybe something like:

import { aMethod, someConst } from "./my-module" with { lazy: true };

function rarelyUsedA() {
  // ...
  import.load "./my-module"; // or maybe "import.load aMethod"
  aMethod();
}

function alsoReallyUsedA() {
  // ...
  import.load "./my-module";
  use(someConst);
}

This would also allow you to mitigate the hazards of doing un-reversible work. E.g. if we just added lazy loading to this module, we can tell module to make sure it loads correctly before the variable is used:

import { tracker } from "./my-module";

function rarelyUsed() {
  // This is the correct place to evaluate the module, not where the tracker variable is used
  import.load "./my-module";
  fs.unlinkSync("./foo.data"); // Cannot be undone
  // We don't want evaluation to happen this late if importing "./my-module" can fail, as
  // in some cases we might wind up in inconsistent states
  tracker.notifyDeleted("./foo.data");
}

@silverwind
Copy link

silverwind commented Apr 5, 2024

As for syntax, I would suggest making use of import attributes which should soon reach stage 4. So instead of

import defer foo from "foo";

it could be

import foo from "foo" with { defer: true };

I think it's important to not needlessly complicate the basic import x from y syntax so we don't end up with stuff like this:

import defer lazy preload foo from "foo"`:

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

No branches or pull requests

3 participants