Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

Rollup: Explicitly set output.exports #1326

Merged
merged 1 commit into from
Jul 30, 2020
Merged

Rollup: Explicitly set output.exports #1326

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions src/core/create_compilers/RollupCompiler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,8 +148,12 @@ export default class RollupCompiler {
}
});

const resp = await bundle.generate({ format: 'cjs' });
const { code } = resp.output ? resp.output[0] : resp;
const {
Copy link
Member

@Conduitry Conduitry Jul 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this now making a new assumption about what version of Rollup you're using? E.g., does this now only work with Rollup >=1? If so, this would be a breaking change.

Copy link
Member

@benmccann benmccann Jul 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good thing to check. This still works with Rollup 1 though. I just tested against the realworld sample app rewound back a few commits to when it was using Rollup 1.15.1

Copy link
Member

@Conduitry Conduitry Jul 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would consider this a breaking change though if this now only works with Rollup 1+. I don't know that the required peer dependency is officially documented anywhere - but this code was specifically here to support both < 1 and >= 1 (added by me in #541).

This seems like a reasonable breaking change to make, however, as Rollup 1 is now relatively old. If so, this should probably be lumped together with some other less-jarring but still-breaking changes.

Copy link
Member

@benmccann benmccann Jul 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I missed that you said >= 1 and just assumed you meant > 1 since 0.x is so old now

So I went back to sveltejs/realworld@0c1c672 and ran it as-is without this change and it worked, but it was relying on an old version of Sapper. Then I upgraded it to the latest version of Sapper (still without this change) and got Error: Cannot find module 'svelte/compiler', so I think we may have broken Rollup 0.x support awhile ago

In either case, I'd be a proponent of merging the other breaking changes that are pending and making the next release 0.28.0

output: [{ code }]
} = await bundle.generate({
exports: 'named',
format: 'cjs'
});

// temporarily override require
const defaultLoader = require.extensions['.js'];
Expand All @@ -161,7 +165,7 @@ export default class RollupCompiler {
}
};

const config: any = require(input);
const config: any = require(input).default;
delete require.cache[input];

return config;
Expand Down