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

Use Babel 7.x; Extract "babel" package; Update "browsers" default #639

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

lukeed
Copy link
Member

@lukeed lukeed commented Sep 7, 2018

Not ready for merge – must resolve pre-existing Jest failures first.


What kind of change does this PR introduce?

Refactor, Feature

Did you add tests for your changes?

No, they're already a mess (again).

Summary

  • Better browser list

  • Upgraded Babel deps from beta to 7.x stable

  • Extracted Preact-specific config into @preact/babel package

  • Applied CLI-specific Babel config changes within CLI:

  • Fixed my typo artifact (old ~> .babelrc)

Does this PR introduce a breaking change?

The default browsers changed, dropping default support for legacy browsers.. so technically, "yes" but this is going to be a major bump already.

The Babel behavior should be unchanged; if anything, it'd be beta to 7.x changes.

This allows non-CLI users to download & designate @preact/babel as a Babel preset.


GitHub killed my original PR notes... wut

@lukeed lukeed changed the title Add "preact/babel Use Babel 7.x; Extract "babel" package; Update "browsers" default Sep 7, 2018
@marvinhagemeister
Copy link
Member

@lukeed Really appreciate this. Was looking for a dedicated preact babel preset to recommend the other day that works with babel v7 👍

@lukeed
Copy link
Member Author

lukeed commented Sep 7, 2018

Me too! I want to slice up what I have on PWA's preset-preact (and 7.x here) into these bite-sized chunks.

Repeating config over & over again across build chains is silly

'src/tests/header.test.js'
'tests/__mocks__/browserMocks.js',
'tests/__mocks__/fileMocks.js',
'tests/header.test.js'
Copy link
Member

Choose a reason for hiding this comment

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

You fixed my PR changes as well 😛

Copy link
Member

@reznord reznord Sep 7, 2018

Choose a reason for hiding this comment

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

Closing #631 since you made these changes

presets: [[require.resolve('@babel/preset-env'), {
"targets": { "node": "current" }
}]]
});
Copy link
Member

Choose a reason for hiding this comment

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

This will break preact.config.js pretty much everywhere, are we gonna change something about that as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, derp. I've always done module.exports and forgot we allowed that. If it were up to me, we'd drop it entirely.

Will add it back.

if (Array.isArray(x) && x[0].includes('@babel/preset-env')) {
x[1].exclude.push('transform-async-to-generator');
}
});
Copy link
Member

Choose a reason for hiding this comment

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

also not sure if this is the best idea, especially if someone wants to extend the config and then async breaks for them

Copy link
Member Author

Choose a reason for hiding this comment

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

If they're overwriting, it gets lost anyway. The extend/overwrite happens after this point.. same as before

Copy link
Member

Choose a reason for hiding this comment

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

but if you extend the config you won't have fast-async because that will override the default one here

Copy link
Member Author

Choose a reason for hiding this comment

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

Fast async isn't included in preact preset because it's not a part of getting Preact to run. It's a CLI feature not a preact requirement

Copy link
Member

Choose a reason for hiding this comment

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

yes, but now if someone makes a custom .babelrc and extends from @preact/babel they'll get errors

Copy link
Member

Choose a reason for hiding this comment

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

in theory you are changing the behaviour from before though, fast-async was in the extendable config from the cli

Copy link
Member Author

Choose a reason for hiding this comment

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

With this package, one can do this:

{
  "presets": [
    "@preact/babel",
    ["@babel/preset-env", {
      "exclude": [
        "transform-async-to-generator"
       ]
     }]
  ],
  "plugins": [
    ["fast-async", { "spec": true }]
  ]
}

And they'd be at the same settings. In ^this state, it's a pointless .babelrc file, but ofc this assumes the user had something worth doing.

Copy link
Member

Choose a reason for hiding this comment

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

is that gonna take the rest of the preset-env config from @preact/babel?

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 @preact/babel is this entire file: https://github.com/developit/preact-cli/blob/6ba1267cb8ffbbeb94705ac6cef246bc075db161/packages/babel/index.js

Everything else you seem to be concerned about is "injected" into @preact/babel by the CLI immediately after loading it.

Copy link
Member

Choose a reason for hiding this comment

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

so the only thing I want to know is if your example .babelrc would basically get turned into this by babel

["@babel/preset-env", {
	"loose": true,
	"modules": false,
    "targets": {
		"browsers": [
			">0.25%",
			"not ie 11'"
			"not op_mini all"
		]
	}
	"exclude": [
		"transform-regenerator",
		"transform-async-to-generator",
		"transform-typeof-symbol"
	]
}]

and our config doesn't get completely overriden

@@ -46,6 +50,7 @@
},
"dependencies": {
"@babel/core": "^7.0.0",
"@babel/register": "^7.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

what about @babel/preset-env? 👀

Copy link
Member Author

Choose a reason for hiding this comment

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

Babel package

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I see what you're saying. It'd resolve but will add to be safe

@lukeed lukeed added this to In progress in Preact-cli 3.0 Sep 8, 2018
@lukeed lukeed mentioned this pull request Sep 15, 2018
@prateekbh prateekbh moved this from In progress to Needed in Preact-cli 3.0 Jan 14, 2019
@prateekbh prateekbh moved this from Needed to In progress in Preact-cli 3.0 Jan 14, 2019
# Conflicts:
#	packages/babel/index.js
#	packages/cli/babel/index.js
#	packages/cli/lib/lib/webpack/transform-config.js
#	packages/cli/lib/lib/webpack/webpack-base-config.js
#	packages/cli/package.json
#	packages/cli/tests/images/create.js
#	packages/cli/yarn.lock
#	yarn.lock
@lukeed
Copy link
Member Author

lukeed commented Jan 25, 2019

Does this pass @ForsakenHarmony inspection now?

@ForsakenHarmony
Copy link
Member

still don't know how I feel about fast-async being excluded

also have babel-plugin-macros in there now 👀

I feel like it's best to just have this as like cli-babel-config or smth

@tomasdev
Copy link

IE11 has more user share than Edge 17/18 as the time of writing this. I would not recommend going with that "better" not so better coverage.

@ForsakenHarmony
Copy link
Member

the difference is, ie 11 keeps us back a lot further than edge, which with that browser list probably doesn't even get special treatment

@tomasdev
Copy link

I completely understand that, then the question I should have asked is: what's the purpose of preact-cli? towards production applications used by corporations, or more towards prototypes future-thinking that don't care about usage?
Of course it's probably none of the extremes, and in the end it's configurable so... up to the owners. I just threw my opinion.

@ForsakenHarmony ForsakenHarmony changed the base branch from next to master March 19, 2019 19:24
@prateekbh prateekbh moved this from In progress to To do in Preact-cli 3.0 Jul 9, 2020
@reznord reznord removed this from To do in Preact-cli 3.0 Jul 10, 2020
@reznord reznord added this to To do in CLI 3.1 Jul 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
CLI 3.1
  
To do
Development

Successfully merging this pull request may close these issues.

None yet

5 participants