Remove gud package#36
Conversation
|
Thank you for contributing :) Can you split the one commit to one commit for the rollup config change and one for the I'll take a proper look at both changes later. |
In this diff I removed gud package which is pure webpackism and requires `global` to be defined. Rollup cannot handle this out of the box. Moreover it's one more commonjs package which need to be transpiled. I inlined commonjsGlobal and added gud like function. Also tweaked rollup config. Migrated to more trusty terser and simplified external function.
| ? global | ||
| : typeof self !== 'undefined' | ||
| ? self | ||
| : {}; |
There was a problem hiding this comment.
Not familiar with this construct. But do we need all fallbacks? If you can point me to some documentation on the matter, that'd be appreciated
Also: Do we need the explicit check for undefined, or would !globalThis do the job as well?
There was a problem hiding this comment.
Thinking about it some more: I'm not a big fan of inlining that polyfill. Users are likely to have the proper polyfill anyway - and the package depends on @babel/runtime too.
There was a problem hiding this comment.
Any not existing global access except typeof variable gives runtime error. I got it here.
https://github.com/rollup/rollup-plugin-commonjs/blob/master/src/helpers.js#L14
Babel runtime does not require from users any polyfilling. gud does require. Yes, webpack does this polyfilling out of the box. Rollup does not. This is really node.js specific thing and should not be in browser library.
There was a problem hiding this comment.
Then we can either directly use globalThis (it's stage-3 now and supported by most browsers) with instructions on how to poly-fill (either through babel/core-js or with a designated polyfill package) or shorten the inlined version:
const commonjsGlobal: any =
typeof globalThis !== 'undefined' // 'global proper'
? globalThis
: typeof window !== 'undefined'
? window // Browser
: typeof global !== 'undefined'
? global // node.jsIf the bare object is used, I expect breakage anyway in some scenarios (like if this library is duplicated across multiple loaded bundles). And if it doesn't break in those scenarios, then a global-ish construct isn't needed anyway.
I'd still prefer not to inline.
There was a problem hiding this comment.
It's not supported by node v8. Polyfills are commonjs modules. Ok, I will remove empty object.
| } | ||
| return !id.includes(path.join(process.cwd(), "src")); | ||
| function isExternal(id) { | ||
| return id.startsWith(".") === false && path.isAbsolute(id) === false; |
There was a problem hiding this comment.
Nitpick: Wrong indentation.
(I should probably add an .editorconfig file.)
There was a problem hiding this comment.
I can rebase if you will setup prettier. By default my editor enforce 2-space indentation.
|
Otherwise LGTM. |
2 participants
In this diff I removed gud package which is pure webpackism and requires
globalto be defined. Rollup cannot handle this out of the box.Moreover it's one more commonjs package which need to be transpiled.
I inlined commonjsGlobal and added gud like function.
Also tweaked rollup config. Migrated to more trusty terser and
simplified external function.