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

Supporting all .js extensions for CJS subpaths #67

Closed
daKmoR opened this issue Jul 16, 2020 · 7 comments
Closed

Supporting all .js extensions for CJS subpaths #67

daKmoR opened this issue Jul 16, 2020 · 7 comments
Labels

Comments

@daKmoR
Copy link

daKmoR commented Jul 16, 2020

if you go to the following URL
https://jspm.dev/@material/mwc-select/mwc-select.js

then it returns a "Not found"

while it's definitely there
https://unpkg.com/@material/mwc-select@0.17.2/mwc-select.js

@guybedford
Copy link
Member

Hey @daKmoR - it's kind of funny this just came up in the Discord this morning. The solution is to use mwc-select without the .js extension as per the standard usage.

The reason for this is that the analysis is usage-based and follows how the package dependents consume the package using require or import. After that there was another bug we fixed this morning, but after that we hit this bug - microsoft/tslib#122.

Feel free to poke that issue.... if it stalls we can try a work around for material itself.

@daKmoR
Copy link
Author

daKmoR commented Jul 16, 2020

hmmm I'm wondering if usage-based is a good thing here 😅 now I suddenly need to remember for which packages to use .js and for which don't 😅

always use .js when using jspm.dev would be a way simpler rule to follow 😅

also confusing is that for lit-element everything works? e.g. all variations work?

I can see the convenience but not having a clear way means I'm never sure if I did it right or not 🙈

@guybedford
Copy link
Member

It's statistical analysis - so what is supported is what is used in majority by the package dependents.

Extension variations are not all iterated because that would have increased the alias module count by almost an order of magnitude resulting in maybe 20 - 50 million more files.

We automatically inline the package contents into its most common variation from a usage perspective. This is why https://jspm.dev/npm:lit-html@1.2.1/directives/class-map contains the source and not https://jspm.dev/npm:lit-html@1.2.1/directives/class-map.js. Since we cannot control how package dependents will import we optimize for the majority.

Note that the package.json "exports" field is always preferable and doesn't have any of these gaps. That is why I spent two years pushing for it and implementing it into Node.js - I would highly recommend posting the package.json "exports" field to packages with a PR to help get them to support it.

@guybedford
Copy link
Member

I can see the convenience but not having a clear way means I'm never sure if I did it right or not 🙈

If you follow the readme / if it works. I thought you were used to programming by now!?

FWIW the CLI release coming in a few weeks supports all variations and doesn't need to make these assumptions.

@guybedford
Copy link
Member

I was thinking a little further about the mental model differences here since I appreciate it seems surprising.

The mental model to ground this is the "exports" field. Now with the exports field it is possible to export with or without a js extension:

{
  "exports": {
    "./feature": "./feature.js"
  }
}

will support without an extension only, while:

{
  "exports": {
    "./feature.js": "./feature.js"
  }
}

would support with an extension. Then we could support both as well:

{
  "exports": {
    "./feature": "./feature.js",
    "./feature.js": "./feature.js"
  }
}

So that is, what is available is down to what the package defines! There is no consistency over whether to use js extensions or not when it comes to subpath exports, in general unfortunately. It is only relative paths and absolute URLs which absolutely have the rule of needing extensions.

Exports also supports folder mappings:

{
  "exports": {
    "./features/": "./features/"
  }
}

will support import 'pkg/features/x.js' for all files in that folder. So would need the extension. Whether it would work for extensionless js files is down to whether your environment supports that (right now Node.js doesn't but there's an active discussion going on if you're interested).

When it comes to packages without "exports", the same model is applied based on the concept of "learning" the exports from usage.

I'm still open to extending to all extension variations on a future build, but hopefully the above clarifies some of the mental model gaps as to why I don't see this as a conceptual oversight as implied by the original response.

@guybedford guybedford changed the title MWC Select "Not found"? Supporting all .js extensions for CJS subpaths Jul 19, 2020
@daKmoR
Copy link
Author

daKmoR commented Jul 22, 2020

@guybedford thx for detailed explanation and the usage of of the exports field is definitely a very good approach. Congrats for getting it into node by the way 💪

So packages that want a certain behaviour can define it themselves and if it's confusing for me as a user I can take it up with them and don't need to bother the CDN.

I was a little hoping we go more the browser way and have file extensions everywhere (except the package bare import) but with exports as an author I can make it happen at least for my packages... maybe it becomes a best practice at some point 🤞

as always top notch information and it help me understand exports a little better 👌

PS: issue can be closed from my side 🤗

@guybedford
Copy link
Member

@daKmoR glad to hear it. This discussion did also push me to post nodejs/modules#535, although I'm weary of putting more complexity into the Node.js resolver.

Importand discussions for sure, thanks for bringing this up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants