-
Notifications
You must be signed in to change notification settings - Fork 781
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
Demos: Add Rollup and Webpack examples #1787
Conversation
3fb2596
to
3a138e7
Compare
982e77b
to
310276f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a few questions but nothing blocking. An interesting approach with separating the bundlers test part.
dir: tmpDir, | ||
entryFileNames: '[name].[format].js', | ||
format: 'umd', | ||
name: 'UNUSED' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When reading a file as ESM, Rollup differentiates between a file that has no exports (i.e. app code), and a file that has exports. When reading a file as CJS, Rollup doesn't understand this, and so it insists on exporting. When using umd
, one of the branches assigns a global to the overall exports
which requires a name. Without the build will fail.
The exports is an empty object, but Rollup wants to put it somewhere, so gave it the name window.
UNUSED
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment in code could be useful here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack. Done in #1791.
for await (const fileName of gRollup) { | ||
console.log('... built ' + fileName); | ||
|
||
if (!fileName.endsWith('.cjs.js')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain this condition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cjs outputs (not to confuse with cjs inputs) are not meant for browser context, as they depend on module.exports
and would throw an uncaught error if you try to load it there. That reminds me, I wanted to add a Node.js test case as well. Right now this file is basically unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment?
310276f
to
e0d6ac0
Compare
…re() Co-authored-by: Michał Gołębiowski-Owczarek <[email protected]>
e0d6ac0
to
580a184
Compare
Previously, `npm run build-dev` and `npm test` could not be run side-by-side due to both using port 4000. I fixed this recently in /demos/bundlers/Gruntfile.js. Let's apply the same fix here too. Ref #1787.
This is in preparation for adding an ESM distribution to QUnit. Before we do so, let's first observe how and what works today:
import QUnit from 'qunit';
import { QUnit } from 'qunit';
import { module, test } from 'qunit';
require('qunit')
In addition, verify that within a given bundler, a mixture of these across multple files, in indirect ways, always refers to the same object, both functionally (extensions to the object are visible to others), and by object identity (strict equality).
Specifically to satisfy Webpack, I've renamed the fixtures that use
require()
from.js
to.cjs
as Webpack refuses to compilerequire()
calls otherwise in a directory with{ type: 'module' }
inpackage.json
. Rollup does not have this restriction.Partially based on jquery/jquery#5429 by @mgol.
Ref #1551.