Added exports to material-ui-icons package.json #26264
Conversation
There was a problem hiding this comment.
There is one breaking change to note about this PR and that is that require('material-ui-icons/x.js') would no longer work with this merged as it would only work without the extension.
As a result it may be sensible to make this PR as part of a major upgrade to this library.
| @@ -64,5 +64,8 @@ | |||
| }, | |||
| "engines": { | |||
| "node": ">=8.0.0" | |||
| }, | |||
| "exports": { | |||
| "./*": "./*.js" | |||
There was a problem hiding this comment.
The main should likely remain included. For backwards compat it can be useful to export the package.json. Perhaps include the ES versions as well. Then you can use null entries to avoid the repeated entries:
| "./*": "./*.js" | |
| ".": "./index.js", | |
| "./package.json": "./package.json", | |
| "./index": null, | |
| "./es/*": null, | |
| "./*": { | |
| "require": "./*.js", | |
| "import": "./es/*.js" |
Details of bundle changes.Comparing: d7993b5...eff69e9 Details of page changes
|
|
We want to do this with some extensive testing with different bundlers. I've heard too many horror stories from introducing |
|
@eps1lon definitely makes sense, but at least the suggestion is there for the next major now. This is also part of the JSPM overrides process to create a PR even if it isn't merged to ensure there is some coordination with upstream (https://github.com/jspm/overrides#criteria-for-merging-an-override). |
|
I definitely hear you. We want to do this in the future. But most of the work needs to be spend in testing. So pushing #23166 forward would really help us. |
|
@eps1lon Webpack 5 supports these features with a high level of parity with Node.js now, we've done quite a bit of testing here. If / when you can try this out against the testing PR feel free to copy me in any issues that arise. |
I know that. I also know that webpack 4 does not support |
|
@jollytoad Note that the active branch is next, not master so we won't be able to merge this one. Regarding rebasing on next, I assume @eps1lon will want to take the lead on this. So maybe we should close this PR? |
|
@oliviertassinari thanks for the follow up, please reference any tracking issue here if closing the PR. We ended up merging quite a few overrides for the projects in https://github.com/jspm/overrides/blob/main/overrides.json which could be a useful starting point for this work further. Would be happy to assist with further upstream review on these if that would help too. |
|
@guybedford We're probably going with a simpler approach considering we only want to ship a single module format: "exports": {
".": "./index.js",
"./*": "./*.js"
},
"type": "module"Though we do want to get all the fixtures in place first (#23166) and then test #26310 against those fixtures. |
|
@eps1lon that sounds good. Are there no subfolders or non-module or non-public JS files at all then? It would still be important to check you are not exporting any internal / private modules that shouldn't be directly imported. Tools can specifically optimize these out. |
We have |
|
@eps1lon you can use {
"exports": {
"./*": "./*.js",
"./utils/*": null,
// or just a specific entry
"./utils/createSvgIcon": null
}
}This ensure they remain private and internal. I understand you're shipping optimized modules, but there can be an encapsulation benefit to this as well. |
That's new to me. I thought |
|
@eps1lon exports are deep includes by default. |
|
Nevermind, we match |
|
Per |
|
And also, |
|
Do you know why the working group thought |
Oh so you were actually part of the modules team. Trying to understand why |
|
@eps1lon this was discussed at length, and you can blame me for suggesting it, but the argument is that these are module specifiers and module URLs which are not file paths, and this maintains symmetry with the RHS replacement without having to bring an entire matching syntax grammar to the resolver specification. This syntax first originated in browser module loaders with resolver configurations like The best way to think about it is that this is a resolver map, acting on module specifier strings. That is there is no globbing or file system checking happening, just string replacement. These strings just happen to map to file paths on a file system. |
|
Yeah makes sense. It just introduces a bit of a pain point for us since we think about Though in the end we can always auto-generate For what it's worth: I think the " |
|
@eps1lon I checked that link you provided but couldn't quite follow what you mean by a difference? Is the issue that you do have lots of other subfolders that you don't want to expose? I checked if there were any improvements we could make in the docs, but we do explicitly call out all these aspects including that it is a glob in https://nodejs.org/api/packages.html#packages_subpath_patterns:
That said we don't currently document the null pattern so I've posted a PR with this at nodejs/node#38724. If you have further exact docs suggestions on this topic do let me know - the feedback is appreciated. |
You do, but it's only after you introduce It's like saying "hey this car is blue" but only later clarifying that your "blue" is actually "red". It's still explicitly saying that what you mean by "blue" is actually "red" but it remains unclear for people opting out earlier. That's at least my perspective.
When we (our team, |
|
@eps1lon I've added an additional note in https://github.com/nodejs/node/pull/38724/files#diff-71a7964a5c439435680952125e31e9dd4fca3a427982e6db86770fd0fbbfde63R361, let me know if that seems clear to you? I will repeat - this is not a matching syntax, it is a string mapping syntax like writing a function Feel free to copy me in for review in future. |
|
@guybedford I have tried to make Material-UI work with JSPM but I couldn't figure it out. First, I have tried v5, it fails on an obscure case.
Then, I have tried v4. I had to add a global variable to support
@eps1lon What do you think about closing this PR as we have #26254 open? |
|
@oliviertassinari I believe it does work if you generate the import map with the |
|
The |
Fixes #26254