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

Treeshaking of arguments leads to SyntaxError #2909

Closed
manucorporat opened this issue Jun 9, 2019 · 8 comments · Fixed by #2911
Closed

Treeshaking of arguments leads to SyntaxError #2909

manucorporat opened this issue Jun 9, 2019 · 8 comments · Fixed by #2911

Comments

@manucorporat
Copy link
Contributor

manucorporat commented Jun 9, 2019

  • Rollup Version: 1.14.4
  • Operating System (or Browser): osx and browser
  • Node Version: 12.0.0

How Do We Reproduce?

https://rollupjs.org/repl?version=1.14.4&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmltcG9ydCUyMCU3QndyaXRlR2VvbWV0cnklN0QlMjBmcm9tJTIwJy4lMkZtYXRocy5qcyclM0IlNUNuY29uc29sZS5sb2cod3JpdGVHZW9tZXRyeSklM0IlMjIlMkMlMjJpc0VudHJ5JTIyJTNBdHJ1ZSU3RCUyQyU3QiUyMm5hbWUlMjIlM0ElMjJtYXRocy5qcyUyMiUyQyUyMmNvZGUlMjIlM0ElMjJleHBvcnQlMjBmdW5jdGlvbiUyMHdyaXRlR2VvbWV0cnkoZ2VvbWV0cnklMkMlMjBvcHRfb3B0aW9ucyklMjAlN0IlNUNuJTIwJTIwcmV0dXJuJTIwd3JpdGVQb2ludEdlb21ldHJ5KChnZW9tZXRyeSklMkMlMjBvcHRfb3B0aW9ucyklM0IlNUNuJTdEJTVDbiU1Q25mdW5jdGlvbiUyMHdyaXRlUG9pbnRHZW9tZXRyeShnZW9tZXRyeSUyQyUyMG9wdF9vcHRpb25zKSUyMCU3QiU1Q24lMjAlMjByZXR1cm4lMjAlN0IlNUNuJTIwJTIwJTIwJTIwdHlwZSUzQSUyMCdQb2ludCclMkMlNUNuJTIwJTIwJTIwJTIwY29vcmRpbmF0ZXMlM0ElMjBnZW9tZXRyeSUyQyU1Q24lMjAlMjAlN0QlM0IlNUNuJTdEJTVDbiUyMiUyQyUyMmlzRW50cnklMjIlM0FmYWxzZSU3RCU1RCUyQyUyMm9wdGlvbnMlMjIlM0ElN0IlMjJmb3JtYXQlMjIlM0ElMjJjanMlMjIlMkMlMjJuYW1lJTIyJTNBJTIybXlCdW5kbGUlMjIlMkMlMjJhbWQlMjIlM0ElN0IlMjJpZCUyMiUzQSUyMiUyMiU3RCUyQyUyMmdsb2JhbHMlMjIlM0ElN0IlN0QlN0QlMkMlMjJleGFtcGxlJTIyJTNBbnVsbCU3RA==

Expected Behavior

It should generate valid JS:

Actual Behavior

It does generate JS with syntax errors:

function writeGeometry(geometry, opt_options) {
  return writePointGeometry((geometry);
}

^ notice the missing closing ) after geometry).

Real world

This issue was first reported by an user of stencil when trying to use the ol (open layers) https://www.npmjs.com/package/ol package.

ionic-team/stencil#1607

Specifically this piece of the code:

function writePointGeometry(geometry, opt_options) {
  return {
    type: 'Point',
    coordinates: geometry.getCoordinates()
  };
}
function writeGeometry(geometry, opt_options) {
  geometry = /** @type {import("../geom/Geometry.js").default} */ (transformWithOptions(geometry, true, opt_options));
  var type = geometry.getType();

  /** @type {GeoJSONGeometry} */
  var geoJSON;
  switch (type) {
    case GeometryType.POINT: {
      geoJSON = writePointGeometry(/** @type {Point} */ (geometry), opt_options);
      break;
    }
 }
}
      geoJSON = writePointGeometry(/** @type {Point} */ (geometry), opt_options);
@manucorporat
Copy link
Contributor Author

Error seems related with acornjs/acorn#136

@manucorporat
Copy link
Contributor Author

The argument's start/end does not include the surrounding parentheses, looking into a solution...

@manucorporat
Copy link
Contributor Author

Looks like this could be a problem for any expression surrounded by parentheses:
Screenshot 2019-06-09 at 16 26 04

This seems like a odd behaviour from acorn

@manucorporat
Copy link
Contributor Author

manucorporat commented Jun 9, 2019

I think the solution could be to set preserveParens: true in the acorn's settings, but that would require to fix a lot of parts of rollup

@lukastaegert
Copy link
Member

I was thinking about that, but I agree it would shake many things up and it would create a non-standard syntax tree. Meanwhile I found a simpler solution, which is to just find the next "," after the argument :) PR upcoming

@lukastaegert
Copy link
Member

Fix at #2910

@manucorporat
Copy link
Contributor Author

@lukastaegert damn! pushed a PR 30 seconds ago
#2911

@manucorporat
Copy link
Contributor Author

same fix!!! haha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants