Skip to content
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

V3 #202

Merged
merged 10 commits into from Jan 16, 2023
Merged

V3 #202

merged 10 commits into from Jan 16, 2023

Conversation

moonglum
Copy link
Member

@moonglum moonglum commented Mar 8, 2022

This PR introduces a few breaking changes. We collect them to have a nice, clean v3 - dropping some things that drag us down along the way.

  • We drop Virtual entirely. It was not used, and adds significant complexity

    • This allows us to collapse the bundle inheritancy hierarchy
  • We drop support for custom extensions

    • I don't think we should have even included it in the first place, I think it is a violation of the faucet philosophy of only supporting "standard code"
    • The file extensions for TypeScript & JSX are added automatically
  • We make ESM the new default format (previously it was IIFE). It is a breaking change, but a very necessary one.

  • Remove an old unused test util and a really old deprecation warning

  • We drop support for Node 14

  • Use ES2015 Syntax in generated code

moonglum and others added 8 commits January 16, 2023 10:35
cf. 3edca46

NB: this constitutes a breaking change
no longer necessary without virtual bundles (cf. previous commit;
separation was originally introduced in
d6c7653)

this also removes duplicated defaults
NB: this constitutes a breaking change
`sourcemaps` has been the preferred option for ages now; cf.
41bacc6

NB: this constitutes a breaking change
long superseded by Dependabot (cf.
51723c1)
NB: this constitutes a breaking change
Co-authored-by: FND <fnd@localhost.localdomain>
@FND
Copy link
Contributor

FND commented Jan 16, 2023

FYI: In an effort to refamiliarize myself with this, I've reviewed every commit here and made some minor changes (mostly tweaking and amending commit messages).

We'll now need to update our test suite to account for latest transpilation changes, but there are also dependencies to be updated (it might make sense to do the latter first).

Copy link
Contributor

@FND FND left a comment

Choose a reason for hiding this comment

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

all dependencies should be up to date now, with corresponding adjustments in our code

@@ -14,6 +14,7 @@ module.exports = function generateBundle(entryPoint, config, cache) {
return bundle.generate(writeConfig);
}).
then(({ output }) => {
output = output.filter(item => item.type !== "asset"); // XXX: simplistic?
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not entirely sure this is correct; it's really hard to keep up with Rollup's API. I mostly guessed based on empiricism and Rollup's change log.

Copy link
Member Author

Choose a reason for hiding this comment

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

🙈

@@ -6,6 +6,9 @@ module.exports = function generateBundle(entryPoint, config, cache) {
let { readConfig, writeConfig } = config;
let options = Object.assign({}, readConfig, {
input: entryPoint,
output: {
generatedCode: "es2015"
Copy link
Contributor

Choose a reason for hiding this comment

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

@moonglum: This doesn't seem to make any difference, at least I didn't have to adjust any tests? (cf. af36dfb)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is weird. For me, the option sounds like something we SHOULD set. But it's weird that we can't find any consequences in the code... I would still vote for keeping the option in here, even though it feels weird.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, fine by me - so ... :shipit: ?

@moonglum moonglum changed the title V3 (WIP) V3 Jan 16, 2023
@moonglum moonglum marked this pull request as ready for review January 16, 2023 16:02
Copy link
Member Author

@moonglum moonglum left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍 We need to adjust the version numbers in the package.jsons - but apart from that, I would say: Let's release them as 3.0.0 👍

@FND FND merged commit af36dfb into main Jan 16, 2023
@moonglum moonglum deleted the v3 branch January 16, 2023 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants