nodejs / modules

Node.js Modules Team
MIT License
413 stars 43 forks source link

Feature: Import JSON without needing asynchronous syntax #114

Closed GeoffreyBooth closed 6 years ago

GeoffreyBooth commented 6 years ago

Updated feature request description:

Imagine code like this in current Node:

const packageJson = JSON.parse(fs.readFileSync('./package.json'));
console.log(`Running version ${packageJson.version}`);

This can also be written as:

const packageJson = require('./package.json');
console.log(`Running version ${packageJson.version}`);

Both of these examples are written without asynchronous syntax on the user’s part: there are no callbacks, no promises, no await. Perhaps Node is doing asynchronous stuff behind the scenes, but the user is unaware of it. require appears to be as synchronous as readFileSync.

This feature request is that a user’s code to import JSON files via ESM should be like these examples: no callbacks, no promises, no await. This feature request doesn’t concern Node’s implementation under the hood, and whether it is synchronous or asynchronous; the request is only that the user’s code be able to be written in a synchronous style.

Use case 48. Part of #100.


Original feature request description:

ESM imports of JSON can be written in a synchronous style:

import { version } from './package.json';
console.log(`Running version ${version}`);

The point of this feature is that users can use the same synchronous style of coding (in the vein of fs.readFileSync as opposed to callback- or promise-based alternatives) when the import is of JSON. Or put another way, users aren’t required to use await or promises or other asynchronous constructs in code in order to import JSON.

Use case 48. Part of #100.

devsnek commented 6 years ago

i don't understand this at all. assuming the "synchronous" part is that its being statically imported, it would be better to then just say that, as there's nothing inherently (a)synchronous about static imports.

if this isn't referring to the static import then it will definitely need to be clarified further.

GeoffreyBooth commented 6 years ago

It’s just that the import doesn’t return a promise, regardless of the module type of what is to be imported. I can import something and then immediately use it in the next line:

import { keys } from 'underscore'; // CommonJS package
const dateKeys = keys(Date);
devsnek commented 6 years ago

so like i said above, maybe this should be rephrased?

GeoffreyBooth commented 6 years ago

What would you have it say? I’m not sure I understand.

The only reason this feature is here is because there was some discussion of handling CommonJS imports via async import(). This feature is a request that the regular ES2015 import statement be usable for CommonJS.

devsnek commented 6 years ago

anything that can be statically imported can be dynamically imported and vice-versa. there is no difference in how they are handled.

neither the use of dynamic or static import informs whether the user agent is actually processing import requests synchronously or asynchronously.

devsnek commented 6 years ago

poorly phrased dupe of #100

GeoffreyBooth commented 6 years ago

Let's not close features without consensus, if you don't mind? As I wrote at the top, this is part of #100. That one is so big that I thought it would be best to discuss its parts in separate threads.

devsnek commented 6 years ago

this issue is literally meaningless. all it says once you degarble it is "allow us to import cjs" which already is what #100 is. specifically stating that you want static imports to work is pointless, and saying "synchronous" is at best a misunderstanding and at worst intentionally misleading.

benjamingr commented 6 years ago

@devsnek I read this issue as automatically creating named exports from JSON files.

benjamingr commented 6 years ago

That said, I'm having a very hard time following all these issues.

GeoffreyBooth commented 6 years ago

this issue is literally meaningless . . . at best a misunderstanding and at worst intentionally misleading.

I would appreciate a more collaborative tone here.

This was an item of its own on the features document. It also seemed to me like a subset of #100, which is also its own item on the features document, but because that one is so huge I’m proposing we split that one up into smaller chunks and then close #100. In the interest of not making unilateral decisions, though, I wanted to leave it to the group to find consensus on how these should be organized. Perhaps that should be a topic for the next meeting.

@benjamingr You are correct, but don’t forget that the feature included CommonJS, not just JSON. I think it’s as simple as it seems, that with regards to importing non-ESM modules, users are allowed to code in a synchronous style (in the vein of fs.readFileSync as opposed to fs.readFile with a callback, or a promise-based version). That’s really it. It’s a cousin of #87, of saying that users can continue using the syntax they’re used to regardless of whether the member to be imported is ESM or CommonJS. I’ve edited the original post to add more detail, please let me know if it’s still unclear.

benjamingr commented 6 years ago

@benjamingr You are correct, but don’t forget that the feature included CommonJS, not just JSON

Then is it possible to open a separate issue for JSON?

GeoffreyBooth commented 6 years ago

@benjamingr Sure, I think I’ll keep this one as JSON (since you’re already discussing JSON above) and I opened #116 for the CommonJS variant.

devsnek commented 6 years ago

@GeoffreyBooth

I would appreciate a more collaborative tone here.

i'm not trying to say the thing this issue describes is a bad idea. i'm saying the way this issue describes it is confusing and at this point i'm going to consider it intentionally inaccurate because i find it hard to believe you haven't read my messages.

so...

please please please,

please please,

please:

stop using the terms "synchronous" and "asynchronous" to describe the syntax of dynamic and static imports.

instead, use the terms "static" and "dynamic"

my quote from above:

anything that can be statically imported can be dynamically imported and vice-versa. there is no difference in how they are handled.

neither the use of dynamic or static import syntax informs whether the user agent is actually processing import requests synchronously or asynchronously.

once again:

anything that can be statically imported can be dynamically imported and vice-versa. there is no difference in how they are handled.

neither the use of dynamic or static import syntax informs whether the user agent is actually processing import requests synchronously or asynchronously.

ljharb commented 6 years ago

That’s not entirely accurate; the spec requires import() to be processed asynchronously (if the module has not yet been evaluated).

devsnek commented 6 years ago

@ljharb thanks for the clarification, but the thing i'm more worried about is "import 'x' is synchronous syntax" which i think is quite a misleading thing to claim.

benjamingr commented 6 years ago

The issue is really "export JSON root properties as names when importing a JSON file".

It has nothing to do with synchronous/asynchrous nor static/dynamic.

benjamingr commented 6 years ago

Also I recommend this section in @2ality's book in case anyone still has doubts about the difference between named exports in ESM and CJS.

GeoffreyBooth commented 6 years ago

This is what I consider to be synchronous syntax:

const packageJson = fs.readFileSync('./package.json', 'utf-8');
console.log(`Running version ${packageJson.version}`);

This is what I consider to be asynchronous syntax:

fs.readFile('./package.json', 'utf-8', function(err, packageJson) {
  console.log(`Running version ${packageJson.version}`);
}
// Or the promise-based equivalent

It’s right there in the name: Sync. I’m referring to the code a user writes, not to anything that might happen under the hood.

I’m feeling bullied in this thread. I don’t need to be ordered what terms to use. I also don’t need to accused of bad faith or intentionally misleading people. I don’t find such comments to be likely to lead to a productive collaboration.

devsnek commented 6 years ago

@GeoffreyBooth both of those examples can be "plugged in" to static import syntax without the user noticing. right now all of node.js's static import syntax is handled by async ops (including something along the lines of promisify(fs.readFile)). that's why i'm requesting that different terminology is used to describe this use case.

requesting that node's loader be synchronous and requesting that it support static import syntax are two very different things, and its confusing if they get mixed up (see my first comment here).

i'm sorry that you feel bullied. i'll try to quiet down a bit.

benjamingr commented 6 years ago

I’m feeling bullied in this thread. I don’t need to be ordered what terms to use. I also don’t need to accused of bad faith or intentionally misleading people. I don’t find such comments to be likely to lead to a productive collaboration.

First of all - thank you for speaking up! It means the world to me that you're calling this out as you see it. I'm sorry you feel that way. Feel free to reach out to me (at benjamingr@gmail.com) or the moderation team directly (report@nodejs.org) if you would like to discuss how that interaction could have gone better so that all parties can learn how to better interact.

I think this was mostly a miscommunication by both parties due to unclear terminology - I apologize for my part in it and I promise to do better and be more attentive in these situations in the future. If you would like me to self-moderate any of my messages - please do feel free to request it either publicly here or in private in either communication channel outlined above.

I think the issue here is:

This is what I consider to be synchronous syntax:

Neither me nor Gus understood that - ESM imports (even import statements like import { foo } from './bar') are asynchronous generally - one of the things ESM does is allow:

import a from 'b';
import b from 'c';
import c from 'd';

To load the modules b, c and d concurrently (which is unlike what CommonJS does) - so I think the confusion is based on the fact that import statements aren't really "synchronous" the way CommonJS is - they're rather a different part of the script life altogether - happening before code starts executing and establishing live-bindings to exports from modules loaded.

Does that help clear why I was confused about what you were writing?

ljharb commented 6 years ago

@benjamingr the loading might happen asynchronously and concurrently, but the evaluation happens linearly, and behaves as it it was synchronous, in the same tick i believe (iow, before any timeouts or Promise resolutions fire). I think this might be part of the confusion?

benjamingr commented 6 years ago

@ljharb to be completely honest I think this is confusing because it is actually complicated - things like live-bindings take time to grok and I think it's very hard to see how confusing it is "from the inside". I'm still not sure about some of the terminology myself to be frank.

Just throwing it out there - Maybe we should do a glossary of the different terminology to get everyone on the same page? I may be being selfish here since I personally am sure I'd learn a lot from it.

bnb commented 6 years ago

@benjamingr +1 to a glossary. I've been toying with this idea for a blog post for a while (as another extension of this thought) but would much rather see this as an official document. Happy to help contribute 👍

GeoffreyBooth commented 6 years ago

Thanks @benjamingr and appreciate it @devsnek. I would also appreciate a glossary.

If there’s any room for misinterpreting the feature request description then it’s clearly not written well enough. Yes, my intent was that the “synchronous” part only apply to the code that a user writes, not to anything that happens behind the scenes on Node’s side. For the purposes of this feature request, whatever Node does under the hood is irrelevant, if the code I write as a user can be written in a synchronous style.

How about I take one more crack at trying to explain this, and @benjamingr or others, please suggest revisions or alternate versions.


Imagine code like this in current Node:

const { version } = JSON.parse(fs.readFileSync('./package.json'));
console.log(`Running version ${version}`);

This can also be written as:

const { version } = require('./package.json');
console.log(`Running version ${version}`);

Both of these examples are written without asynchronous syntax on the user’s part: there are no callbacks, no promises, no await. Perhaps Node is doing asynchronous stuff behind the scenes, but the user is unaware of it. require appears to be as synchronous as readFileSync.

This feature request is that a user’s code to import JSON files via ESM should be like these examples: no callbacks, no promises, no await. This feature request doesn’t concern Node’s implementation under the hood, and whether it is synchronous or asynchronous; the request is only that the user’s code be able to be written in a synchronous style.

ljharb commented 6 years ago

To clarify, this could also be written as:

const pkg = require('./package.json'); // eg, `import pkg from './package.json';`
const version = pkg.version;
console.log(`Running version ${version}`);

just to remove any distraction about named imports, which are unrelated to this feature.

benjamingr commented 6 years ago

Though in this particular case since JSON is data and not code we actually could provide named exports 😅

I think we should, by default, regardless of whether or not we do it for CJS. Namely because I don't know of any reasons why not to - am ready to be convinced though.

targos commented 6 years ago

There's already a discussion about named exports for JSON at https://github.com/nodejs/node/issues/20494

GeoffreyBooth commented 6 years ago

I’ve updated the original post. Please let me know if it can be improved further.

guybedford commented 6 years ago

I think this seems like a subfeature of #116 at this point to me. Let me know if I'm missing anything here.

GeoffreyBooth commented 6 years ago

The syntax part is identical, but the call-out of JSON is different. Perhaps “JSON can be imported into ESM” deserves its own feature.