Skip to content
This repository has been archived by the owner on Jan 6, 2018. It is now read-only.

Use recast to strip functions #7

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

Use recast to strip functions #7

wants to merge 2 commits into from

Conversation

Vijar
Copy link
Contributor

@Vijar Vijar commented Feb 22, 2015

I got this working with recast. Mainly took inspiration from fixmyjs.

This would fix #4

However, this is now backwards incompatible since we can no longer strip console.log or any other CallExpressions that are considered MemberExpressions. We can only strip Identifiers such as 'debug' or 'assert'.

I actually don't think this loader should be stripping console.log statements anyway, so I actually don't mind the breaking version.

@gpbl, if you can test this branch for your code and give me a +1, I will merge.

@yahoocla
Copy link

CLA is valid!

@coveralls
Copy link

Coverage Status

Coverage decreased (-8.33%) to 91.67% when pulling 98d8b2d on recast into 5020e42 on master.

@gpbl
Copy link
Contributor

gpbl commented Feb 22, 2015

Thanks @Vijar trying it now: I have an issue with recast and babeljs, let me investigate first :-)
I do agree console.log shouldn't be stripped by this loader, it's also a job for the Uglifier.

@coveralls
Copy link

Coverage Status

Coverage decreased (-4.11%) to 95.89% when pulling 5b932da on recast into 5020e42 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-4.11%) to 95.89% when pulling 5b932da on recast into 5020e42 on master.

@gpbl
Copy link
Contributor

gpbl commented Feb 22, 2015

@Vijar I believe recast introduces a new level of complexities :-)

This code with an inline jsx comment didn't work with the strip-loader when running the babeljs-loader first:

render() {
  return (
    <p>
     { /* inline jsx comment */ }
    </p>
  );
}

recast throws an error:

Module build failed: Error: Comment location overlaps with node location

because babel puts the comment on the bottom of the file (babel/babel#459, babel/babel#672). So recast is safer than a regexp, on the other side it put some limitations we should be aware of.

@Vijar
Copy link
Contributor Author

Vijar commented Feb 22, 2015

@gpbl, I actually did try to preserve comments but recast doesn't play nice with comments at all! :(

I think we may have to do what fixmyjs guys did, create a v2.0 release with recast because it is safer, but still maintain a legacy version which still uses your wider regexp PR. We can address the pros/cons of each in the README.

@gpbl
Copy link
Contributor

gpbl commented Feb 23, 2015

@Vijar 👍 thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not working with (
4 participants