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

Minor bug with comment handling #12

Closed
evan-scott-zocdoc opened this issue Dec 22, 2017 · 6 comments
Closed

Minor bug with comment handling #12

evan-scott-zocdoc opened this issue Dec 22, 2017 · 6 comments
Labels

Comments

@evan-scott-zocdoc
Copy link

https://github.com/Andarist/babel-plugin-annotate-pure-calls/blob/master/src/index.js#L6-L10

I have a comment in my code that looks like this:

/* eslint-disable babel/new-cap */

and it seems to be triggering this error:

ERROR in [redacted].js
Module build failed: TypeError: [redacted].js: Cannot read property 'some' of null
    at isPureAnnotated ([redacted]/node_modules/babel-plugin-annotate-pure-calls/lib/index.js:19:26)
    at annotateAsPure ([redacted]/babel-plugin-annotate-pure-calls/lib/index.js:26:7)
    at PluginPass.callableExpressionVisitor ([redacted]/babel-plugin-annotate-pure-calls/lib/index.js:111:3)

@Andarist
Copy link
Owner

1st issue reported by the community member 🎉

I guess leadingComments property is not set on the node (speaking totally from my gut, im in a travel on a mobile, havent checked the source code). I can guard against that ofc, but it's pretty interesting why this is not set. Thought it's always there, I remember someone mentioning something about similar case on Babel's Slack - need to dig into this.

Could you paste in the minimal repro file? Would love to examine how Babel handles it and of course to write a test for this.

@evan-scott-zocdoc
Copy link
Author

I think this would be enough?

const test = 'hello world';

/* eslint-disable babel/new-cap */
export default test;

@Andarist
Copy link
Owner

Andarist commented Dec 23, 2017

Fixed on the master (targeting babel@6) and released. Didn't make any changes to the babel-7 branch, because this deserves a fix upstream in babel itself IMHO. The situation should be normalized, even babel's typings does not make those fields optional.

Thanks for the report! Would love to hear how (where?) do you use the plugin.

@Andarist Andarist added the bug label Dec 30, 2017
@evan-scott-zocdoc
Copy link
Author

Thanks @Andarist, I'll try out the change.

I'm attempting to use this plugin as a way of overcoming the fact that webpack 3's treeshaking is broken.

@Andarist
Copy link
Owner

Andarist commented Jan 2, 2018

@evan-scott-zocdoc If you want to use it on webpack's output I guess you are using it on application+lib bundle, due to the fact that it's probably a much bigger codebase than a single library I advise caution - but ofc you probably know that this is somewhat a little bit dangerous transform.

Tree-shaking is prevented mainly in cases of:

  • function calls, heavy fp usage (compose, curried functions etc) - latter should be quite safely fixed by this plugin
  • static properties, most common use case being classes - this one is out of scope of this plugin, I'm working (nearly finished) on the PR to babel which should improve the situation for those (will be available in future babel@7-betas)

And ofc enabling ModuleConcatenationPlugin can improve things quite a lot (worth noting this for other readers, you probably already know what :))

@evan-scott-zocdoc
Copy link
Author

evan-scott-zocdoc commented Jan 3, 2018

And ofc enabling ModuleConcatenationPlugin can improve things quite a lot

Definitely, we've been using that for a few months.

The specific target I'm trying to treeshake more efficiently is the so-called "re-export file", e.g. an index file that imports from many other files and re-exports under different names. In many cases, a person will import that index and destructure out a specific export, but all the other exports end up coming along for the ride anyway.

// index.js
export { default as a } from './a';
export { default as b } from './b';
export { default as c } from './c';

// some other file
import { a } from 'index.js';

// ideally b and c would be treeshaken

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

No branches or pull requests

2 participants