typescript error with address-formatter 4.0.2
Closed this issue · 4 comments
I updated my dependencies today and noticed that my server started throwing errors related to address-formatter:
I had already a while back updated my code to use imports instead of require:
import addressFormatter from '@fragaria/address-formatter'after the new update (version 3.0.1 -> 4.0.2), addressFormatter was suddenly undefined
I then checked the latest commits and saw that all of the projects require had been changed to imports, which is great so thank for that, but at the same time the exports of address-formatter went from default exports to named exports.
Then I changed the import to a named import:
import { addressFormatter } from '@fragaria/address-formatter'I'm using Typescript for my backend code and I noticed I had a new error when building:
error TS2305: Module '"@fragaria/address-formatter"' has no exported member 'addressFormatter'.I created a sandbox to reproduce the error: https://codesandbox.io/s/suspicious-wind-7ncf7w?file=/index.ts
So I wonder why this change was made now, especially because the project only had a single export which is addressFormatter, if the project had gone from one export to multiple I would understand the change from one default to multiple named exports, but this doesn't seem to be the reason. Again I think the internal changes from require to imports are good, but I have a doubt changing the exports was a good idea, especially because they break backwards compatibility with the previous versions of the package.
Another problem is that because of this change the index.d.ts typescript typings are now wrong, which means typescript users will get errors when trying to compile, even developers that don't use typescript still need to update their code that was import addressFormatter to import { addressFormatter }. The second disadvantage I see is that now in the browser you need to write addressFormatter.addressFormatter.format() instead of the shorter version addressFormatter.format() we had in the previous major version.
So I went ahead and did a PR #673 that updates the index.d.ts typings file but I also reverts back to default exports (but I kept all the other updates you did). This solves the typescript bug mentioned above and makes the transition from version 3.x to the new version easier for developers, as they can now upgrade without touching a single line of their code.
This is why I wanted to ask you @JirkaChadima, could you please have a look at my PR and maybe consider accepting it? But maybe there is a reason for the changes that I'm not aware of, in which case the index.d.ts would still need to get fixed to replect the change from default to named exports?
I used my updated version in my own project and had no problems anymore (without changing a line of my code)
I also wrote a test to see if the UMD version for browsers still works:
<html>
<head>
<script src="./dist/umd/address-formatter.js"></script>
</head>
<body>
<div id="output"></div>
<script>
function formatStart() {
const formatted = window.addressFormatter.format({
"houseNumber": 301,
"road": "Hamilton Avenue",
"neighbourhood": "Crescent Park",
"city": "Palo Alto",
"postcode": 94303,
"county": "Santa Clara County",
"state": "California",
"country": "United States of America",
"countryCode": "US",
});
console.log(formatted);
const outputDiv = document.getElementById('output');
outputDiv.innerHTML = formatted;
};
formatStart()
</script>
</body>
</html>I have now split the two solutions into two seperate PRs, so now it is easier for you, if you want to accept one or the other solution (instead of having both solutions in one PR as I did earlier)
so PR #673 is the solution where I go back to default exports (in that case no type declaration changes in index.d.ts are needed) (I initially had the declaration fix in that PR too, but removed it now)
and the new PR #676 is the other solution that keeps address-formatter version 4.0.2 as is, but adds a namespace to the types declarations in index.d.ts to fix the typescript error "Module has no exported member" as shown in this codesandbox: https://codesandbox.io/s/suspicious-wind-7ncf7w?file=/index.ts
@chrisweb Thank you for this. I'm not a big typescript guy and tbh I got a little bit lost in all of the flavours of requires/imports/exports that are currently out there. I'll take a look at this tomorrow or some time next week.
@JirkaChadima Yes, no problem and no hurry, also thx for taking your time to check this out with me. Agree, it is the same for me, I sometimes feel completely lost when trying to figure out how to create versions of a package that work for everyone and brake the least amount of existing implementations. I guess the transition from require to import in nodejs the last few years has been a nightmare for a lot of developers (not blaming anyone, especially not the nodejs team, I can imagine how hard it was to come up with a plan for the transition from cjs modules via “require” to es modules via “import” and at the same time not break the entire web and also trying to be as close to what happened in browser implementations, the typescript ecosystem and in module loaders like require.js, system.js, webpack and all the others) as one can read in the nodejs modules roadmap: https://github.com/nodejs/modules/blob/main/doc/archive/plan-for-new-modules-implementation.md
But back to the two PRs I did, they actually try to change as few things as possible, but either of the two is needed to fix the imports when using typescript (vanilla js works fine)
The first PR does a rollback and goes back to using default exports because this what this library used before (up to version 3.0.1), this fixes imports in typescript
My second PR keeps the transition from default to named exports but fixes the typescript declaration file (I hope I did it right, I tested it and it works for me, but not sure I did it the best way there is, I’m also not a typescript guru), by adding a namespace to index.d.ts we tell typescript how to find the format function that is inside of the addressFormatter object
However no matter which of the two PRs you chose, even after those changes “import” is still going to load the common js (cjs) module, that is specified via the field named “main” in the package.json, (same as for require that will also use the path from the “main” field), for browser js code that uses a module bundler it is the UMD module that gets loaded as defined in the package.json “browser” field, BUT the third field “module” that is currently in the package.json will never get used (this field references the “es module” path), this is because the module field for package.json was something that at some point got considered but it never got implemented (neither nodejs nor npm did ever add support for it, webpack 2 was using it but dropped support for it later on). This stackoverflow answer has a good recap: https://stackoverflow.com/a/42817320/656689
So besides fixing imports in typescript, if you want to go a step further, I think it is safe to remove the “module” field from package.json by altering the rollup configuration file to not create / populate it anymore
Now if “module” is not being used, how can you specify both the path of common js (cjs) as well as “es modules”, well that is done via the package.json field “exports”, the documentation can be found here: https://nodejs.org/api/packages.html#exports, this field can have conditional exports which tell both “require” (to use the common js (cjs) version) and “import” (to use the es module version) by defining a different path for both, there are some good examples of different patterns for this in the webpack documentation: https://webpack.js.org/guides/package-exports/#target-environment-independent-packages, support for “exports” got introduced in Node.js 12+
A bit of nodejs history: From version 8.5.0 (MDN says it was version 8.5.x but I saw a blog post from the nodejs module team that says it was 8.9.x https://nodejs.medium.com/announcing-a-new-experimental-modules-1be8d2d6c2ff) until version 12.0.0 (exclusive) nodejs had an early version of support for "import" but the feature was behind an --experimental-modules flag
Then in nodejs 12+ they introduced a new way of supporting import and also the exports field in package.json but again this was behind the --experimental-modules flag (the flag name did not change), however there were some conditions: es modules had to either have a filename ending in .mjs, or the nearest parent package.json had to contain a field called "type" that would have as value "module", the "type" field would also accept the value "commonjs" if the file path in "main" is a commonjs file, documentation: https://nodejs.org/api/all.html#all_packages_type
Finally in version 13.2.0 they dropped the need to use the experimental flag --experimental-modules, but still es modules had to have a .mjs extension or you had to use the field "type" to define that your package uses “es modules”
The source for this history lesson is here in the compatibility table section of the MDN import documentation: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import
TLDR: I hope I didn’t lose you because of all this blabla ;) as a recap, neither nodejs nor typescript will today use the “es module” version of this package as it is defined in the obsolete “module” package.json field, there are two ways to make the package use the “es module”, the first one is to put the path of the “es module” into the package.json “main” field, but this means that you drop the commonjs (cjs) module and “require” support for nodejs users in favor of “import” only, the second solution is to keep the cjs version path in the “main” field but also add the field “exports” to the package.json file and put the path to the “es module” in there (as a replacement for the “module” field currently being used), now this is the theory, I did not test this yet, but if you want I could do a PR and run some tests, let me know what you think
oh jeez, this is complicated :D I like the default export, so I will go forward with #673
I'm not brave enough to remove more things, such as the module field at the moment. Thank you for the thorough explanation and the PRs, I'll cut the 5 release back with default exports