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

Allow bundling browserify bundles by replacing free requires with undefined #6260

Merged
merged 4 commits into from May 14, 2021

Conversation

mischnic
Copy link
Member

@mischnic mischnic commented May 10, 2021

Closes #3933, Closes #4634, Closes T-1002

This replaces typeof require == "function" && require (and commutative equivalents, or with ===) with undefined, so that browserify bundles don't throw. require is never actually called, but still assigned to a local variable.

Previously, it was replaced with "function" == "function" && require.

This replaces free requires (so everything except typeof require and require("some...string") with undefined.

@height
Copy link

height bot commented May 10, 2021

This pull request has been linked to and will mark 1 task as "Done" when merged:

  • T-1002 (exists with old scope hoisting) Browserify-built bundles are incompatible (unlink task)

💡Tip: You can link multiple Height tasks to a pull request.

@parcel-benchmark
Copy link

parcel-benchmark commented May 10, 2021

Benchmark Results

Kitchen Sink 🚨

Timings

Description Time Difference
Cold FAILED -0.00ms
Cached FAILED -0.00ms

Cold Bundles

No bundles found, this is probably a failed build...

Cached Bundles

No bundles found, this is probably a failed build...

React HackerNews 🚨

Timings

Description Time Difference
Cold FAILED -0.00ms
Cached FAILED -0.00ms

Cold Bundles

No bundles found, this is probably a failed build...

Cached Bundles

No bundles found, this is probably a failed build...

AtlasKit Editor 🚨

Timings

Description Time Difference
Cold FAILED -0.00ms
Cached FAILED -0.00ms

Cold Bundles

No bundles found, this is probably a failed build...

Cached Bundles

No bundles found, this is probably a failed build...

Three.js 🚨

Timings

Description Time Difference
Cold FAILED -0.00ms
Cached FAILED -0.00ms

Cold Bundles

No bundles found, this is probably a failed build...

Cached Bundles

No bundles found, this is probably a failed build...

Click here to view a detailed benchmark overview.

@devongovett
Copy link
Member

Would it be simpler to only look for require as a free identifier? Right now we don't do anything with that. If we replaced it with undefined then that might also fix this. I can't think of a case where a free require would work anyway.

@mischnic
Copy link
Member Author

It now replaces free requires instead. That wasn't really possible before the swc transformer because there was no __parcel__require__ for the runtimes.


return node.fold_children_with(self);
fn fold_unary_expr(&mut self, node: ast::UnaryExpr) -> ast::UnaryExpr {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't fold_expr already take care of this? It should run before fold_unary_expr I think.

Copy link
Member Author

@mischnic mischnic May 13, 2021

Choose a reason for hiding this comment

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

UnaryExpr is an enum variant of Expr, so Expr is visited before the UnaryExpr.

Do you mean I should move the logic from fold_unary_expr to fold_expr?

Copy link
Member

Choose a reason for hiding this comment

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

I mean, we already handle UnaryExpr inside the fold_expr visitor, and return without visiting children, so I'm not sure why this additional visitor is needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean, we already handle UnaryExpr inside the fold_expr visitor

I think you are looking at hoist.rs? All of my current changes are in dependency_collector.rs now

Copy link
Member

Choose a reason for hiding this comment

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

Oh right 🙈

@wbinnssmith wbinnssmith changed the title Allow bundling browserify bundles by replacing typeof require == "function" && require Allow bundling browserify bundles by replacing free requires with undefined May 13, 2021
@devongovett devongovett merged commit d0e8df6 into v2 May 14, 2021
@devongovett devongovett deleted the swc-browserify branch May 14, 2021 03:07
}
}

node.fold_children_with(self)
}

Copy link

Choose a reason for hiding this comment

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

Seems like fold_member_expr is missing.
All passes touching identifier should implement it and fold prop only if computed is true.

Copy link

Choose a reason for hiding this comment

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

Seems like a cause of #6323

Copy link

Choose a reason for hiding this comment

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

I was wrong. Sorry for the mistake.

lettertwo added a commit that referenced this pull request Jun 4, 2021
…raph

* bdo/buffer-backed-graph: (37 commits)
  Improve resolver performance (#6328)
  v2.0.0-beta.3.1
  Add it to the nightly release workflow as well
  Configure jemalloc page size for M1 (#6314)
  Update swc (#6307)
  Fix parcelDependencies
  v2.0.0-beta.3
  Update swc (#6289)
  Remove old AdjacencyList, serialize EfficientGraph in Graph, update BundleGraph to use new functions
  Babel ast location, diagnostic, and source location remapping (#6238)
  update contributing guide (#6293)
  Update sourcemap to rc-1.0 (#6279)
  Serve nearest index.html in case the applications has multiple index files (#6250)
  Allow bundling browserify bundles by replacing free requires with undefined (#6260)
  Upgrade flow to 0.151.0 (#6281)
  Use local require for `@babel/core` and `postcss` (#6264)
  Resolve helpers relative to JS transformer (#6278)
  Pure comment for $parcel$interopDefault (#6271)
  Don't transpile spread in JSX with modern targets (#6274)
  Update TS validator assertions (#6272)
  ...
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.

Uncaught ReferenceError: require is not defined
5 participants