Skip to content

Remove fast-async - #1347

Merged
ForsakenHarmony merged 11 commits into
preactjs:masterfrom
teodragovic:remove-fast-async
Oct 6, 2021
Merged

Remove fast-async#1347
ForsakenHarmony merged 11 commits into
preactjs:masterfrom
teodragovic:remove-fast-async

Conversation

@teodragovic

@teodragovic teodragovic commented Jul 25, 2020

Copy link
Copy Markdown
Contributor

What kind of change does this PR introduce?

Remove fast-async from dependency list in favor of using transform-async-to-generator

Fixes #1591

Did you add tests for your changes?

No

Summary

Going off this comment by @developit I jumped on it as a chance to get rid of moderate security vulnerability after preact-cli install.

Does this PR introduce a breaking change?

I don't think so?

@prateekbh
prateekbh requested a review from developit August 5, 2020 20:31

@prateekbh prateekbh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

some questions

excludedPlugins: ['BabelEsmPlugin', 'InjectManifest'],
beforeStartExecution: (plugins, newConfig) => {
const babelPlugins = newConfig.plugins;
newConfig.plugins = babelPlugins.filter((plugin) => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this makes sure 'fast-async' are not added, why remove this?

@teodragovic teodragovic Aug 6, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I did a serach for fast-async in code and figured if I'm removing it as dependency any related code would be safe to remove also. Unless goal was to also prevent users from adding it via custom babelrc?

Comment thread packages/cli/lib/lib/babel-config.js Outdated
],
],
plugins: [
require.resolve('babel-plugin-transform-async-to-promises'),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

in modern mode we don't need this. All modern browsers support async/await. Wouldn't adding this here add it for both modern and legacy mode?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

True. Removed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hm, now the tests are failing again. I'll admit this task proved more complex than I thought so I would welcome some guidance around it.

@prateekbh prateekbh added this to the 3.1 milestone Aug 5, 2020
@developit

Copy link
Copy Markdown
Member

One thing I'd like to consider before we jump to merging this: in the case where esm output is not enabled, outputting regenerator is a bad default. Perhaps we could have that case use babel-plugin-transform-async-to-promises?

@teodragovic

teodragovic commented Aug 13, 2020

Copy link
Copy Markdown
Contributor Author

outputting regenerator is a bad default.

Due to the amount of code transpiled or is there more to it? My (very shallow) understanding is that Babel would adjust output based on browserslist config (either default or provided by the user) so if deeper browser support is needed (ie. no esm) it would transpile all the way down to promises. I guess then the problem might be disabling esm but not updating browserslist?

I'm also interested to learn how #1116 plays into this?

@peterbe

peterbe commented Sep 23, 2021

Copy link
Copy Markdown

@teodragovic Any chance you can take a look at those merge conflicts with the main branch and give this PR some fresh air?
Also, I thing you can add Fixes #1591 into the original PR comment.

@teodragovic
teodragovic requested a review from a team as a code owner September 24, 2021 15:43
@changeset-bot

changeset-bot Bot commented Sep 24, 2021

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: b31fa58

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
preact-cli Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

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.

Cloud Firestore v9 build error with preact-cli

5 participants