fix(types): Enable plugin to be imported under ES6 #316
Conversation
lobsterkatie
force-pushed
the
kmclb-fix-exports-for-default-import
branch
from
a76a01d
to
a3465f9
Compare
| "peerDependencies": { | ||
| "@types/webpack": "^4.0.0 || ^5.0.0" | ||
| }, |
There was a problem hiding this comment.
I'm not sure about this. This way we will make everyone install it, even when they are not using TypeScript. Otherwise, we'll warn every time they don't.
There was a problem hiding this comment.
Warning is fine IMO. But, when does one install the webpack plugin and not webpack?
There was a problem hiding this comment.
Never, but those are types, not webpack itself.
There was a problem hiding this comment.
Yeah, I debated about that. I can't believe we put a man on the moon more than half a century ago and still haven't solved this problem. Either we:
-
make it a hard dependency ( which is what the TS docs would have us do), and plain JS people install a package they don't need, or
-
make it a peer dependency, and plain JS people get warnings which don't apply to them, or
-
make it neither, and TS people get yelled at by their linter and/or compiler unless they just happen to already have the relevant types installed.
Which is the least bad, do we think?
There was a problem hiding this comment.
The first one is a no-go for me, and I rather the third over the second although not strongly opinionated against the second one.
- Why should JS people get warnings related to TS?
- TS people know they sometimes need to install additional modules for the types, so this shouldn't be a big deal.
There was a problem hiding this comment.
I'd also go with 3rd. If you are using TypeScript and Webpack, there's 99,99% chance that you already have those types installed.
There was a problem hiding this comment.
TS people know they sometimes need to install additional modules for the types, so this shouldn't be a big deal.
Yeah, but that's generally for their own dependencies, not transitive ones. I agree it's not the world's biggest blocker, and people will figure it out, so in this case I'm happy to go with option 3, especially since you're right, @kamilogorek, it's not like webpack is obscure and at least some folks will already have the types. (I'm less convinced it's everyone, though. If you're using our nextjs SDK and don't have any custom webpack config other than what you pass to our plugin, there's no reason you'd already have them.) So I can pull it.
As a general principle, though, and given just how popular TS is, it's still just amazing to me that npm/yarn haven't adapted to what has to be a pretty common situation/problem.
There was a problem hiding this comment.
As a general principle, though, and given just how popular TS is, it's still just amazing to me that npm/yarn haven't adapted to what has to be a pretty common situation/problem.
Not sure this is only an npm/yarn thing, but also from the npm registry? But yes, there should be a solution to this.
There was a problem hiding this comment.
Not sure this is only an npm/yarn thing, but also from the npm registry?
Not sure what you mean.
There was a problem hiding this comment.
The npm registry is npmjs.com, and then the npm tool which you use to manage the modules in the registry. Anyways, I haven't dived into the registry and I don't know where the solution should be coming from, just giving my thoughts.
| "peerDependencies": { | ||
| "@types/webpack": "^4.0.0 || ^5.0.0" | ||
| }, |
There was a problem hiding this comment.
Warning is fine IMO. But, when does one install the webpack plugin and not webpack?
|
Feel free to merge it |
|
So I did remove the peer dependency... and then forgot to push the change before I merged. 🤦🏻♀️ It's included as a separate commit here. |
|
correct link eff7c9d |
) This lets us take advantage of getsentry/sentry-webpack-plugin#316.
Because the webpack plugin is written in vanilla JS (rather than TS), we have a manually-generated type declaration file, where we default-export the
SentryCliPluginclass. We also have a file (cjs.js) which is meant to make the plugin play nicely with CommonJS imports. And if the user is indeed using CommonJS imports (const SentryWebpackPlugin = require('@sentry/webpack-plugin')), that all works great.But if you try to do ES6 imports, the only form which gets you anything besides
undefinedisimport * as SentryWebpackPlugin from '@sentry/webpack-plugin'.* As far as TS is concerned, though, that creates a namespace rather than a class, and so it will refuse to let you callnew SentryWebpackPlugin, instead telling you thatSentryWebpackPluginis not contructable. One solution is to useesModuleInterop, but that's gotten us into trouble before and in fact breaks some of our own SDK code.*The other two options are
import SentryCliPlugin as SentryWebpackPlugin from '@sentry/webpack-plugin'andimport { default as SentryWebpackPlugin } from '@sentry/webpack-plugin', both of which let you callnewon them, but neither of which currently actually gives you anything other thanundefined).The alternative (which is what's done in this PR) is to do two things:
Use the older, more universal
exports = ...syntax in our type declaration file rather than our currentexport default ...syntax (which, according to the TS docs, actually won’t work unlessesModuleInteropis set totrue). This then necessitates moving the options interface into a namespace for the module.Explicitly create an export under the name
default, so that the two alternatives above actually come up with something rather than being undefined.This shouldn't affect any current imports, as the first change just moves to an equivalent syntax, and the second change is purely additive.