search

jkrems / proposal-pkg-exports

Proposal for Bare Module Specifier Resolution in node.js
MIT License
129 stars 14 forks source link

Update to use `./` root #20

Closed guybedford closed 5 years ago

guybedford commented 5 years ago

As discussed in https://github.com/jkrems/proposal-pkg-exports/issues/18.

This also resolves the main very nicely too!

GeoffreyBooth commented 5 years ago

So this means that all exports LHS would need to start with ., correct? What if someone starts with / or a letter?

jkrems commented 5 years ago

So this means that all exports LHS would need to start with ., correct? What if someone starts with / or a letter?

My assumption is: If the LHS starts with anything but ., it's either ignored w/ a warning or an error. I personally prefer to error.

GeoffreyBooth commented 5 years ago

it’s either ignored w/ a warning or an error. I personally prefer to error.

We should mention this in the proposal I think, if it’s not there already.

This is the risk of using . as our placeholder for package name, as opposed to something more outlandish like <package-name> (not that I’m recommending that). With ., people might expect this to behave similarly to file paths, where ./file and file are equivalent. I guess the question is whether that potential confusion is more or less of an issue than the current proposal’s empty string and “string concatenation when it looks like a file path” confusion.

guybedford commented 5 years ago

My assumption is: If the LHS starts with anything but ., it's either ignored w/ a warning or an error. I personally prefer to error.

I used to think the same, but what was discussed in the import maps proposal was to always provide a silent failure or warning over a hard error because an error would stop any forwards-compatibility with any new syntax support added in future.

jkrems commented 5 years ago

Given the slight chance of confusion, I think we'd definitely want a warning so people don't try { "foo": "bar.js" } and wonder why nothing is happening. It's a bit different with import maps where I would be surprised if people realistically would write them by hand.

guybedford commented 5 years ago

Can we get this merged? I actually already implemented this approach...

GeoffreyBooth commented 5 years ago

I was hoping we could add a note about how . is just a placeholder for the package name, and not equivalent to file system paths; hence why "./dist/" works but "dist/" doesn’t.

guybedford commented 5 years ago

@GeoffreyBooth sure can you suggest an edit for that?

jkrems commented 5 years ago

I was hoping we could add a note about how . is just a placeholder for the package name, and not equivalent to file system paths; hence why "./dist/" works but "dist/" doesn’t.

I'm going to go ahead and merge this since it will reflect the current state of the discussions a bit better. We can land additional clarifications as follow-ups.