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

Change public exports shape #53

Merged
merged 3 commits into from
Aug 25, 2020

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented Jun 2, 2020

Why?

Improving tree-shakeability of this package. Usage of static properties like this usually retains the containing object (in this case a function) even if it stays unused. This matters when tabbable is being used as transitive dep.

Partially fixes #39

⚠️ this is a breaking change

Copy link
Member

@stefcameron stefcameron left a comment

Choose a reason for hiding this comment

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

@Andarist Thanks for this PR! I'm taking over from David since he isn't maintaining this, or focus-trap, or focus-trap-react anymore. I'm all for making this module more compatible with tree-shaking. I'd just request to not use default exports at all. If you're OK with that, please also update the README changes you're making in order to reflect that.

Also, what, in your opinion, is still missing in this to fully address #39?

src/index.js Outdated
Comment on lines 172 to 173
export default tabbable;
export { isTabbable, isFocusable };
Copy link
Member

Choose a reason for hiding this comment

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

Since we bundle as CJS also, I'd like to avoid potential bundling issues and not have a default export. Could we just export all functions as export { tabbable, isTabbable, isFocusable }?

Rollup actually considers it a bad practice to mix named and default exports in the same module. I'd rather avoid them. 😄

@Andarist
Copy link
Contributor Author

Also, what, in your opinion, is still missing in this to fully address #39?

I would have to review this, can do that later this week.

Since we bundle as CJS also, I'd like to avoid potential bundling issues and not have a default export. Could we just export all functions as export { tabbable, isTabbable, isFocusable }?

Sure, you can go with named exports only route.

@Andarist
Copy link
Contributor Author

Ok, I've adjusted the PR according to your comments - please take a look.

Also, what, in your opinion, is still missing in this to fully address #39?

  1. /* #__PURE__ */ comment before a call here
  2. tools don't want to remove top-level getters because of the potential side-effects and, unfortunately, there is no perfect way to make those treeshakeable. There are only quirk solutions for it - so it depends if you care about having this tree-shakeable. If yes, then I could give some guidance on how to fix this statement.

Copy link
Member

@stefcameron stefcameron left a comment

Choose a reason for hiding this comment

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

This is looking good, thanks for updating to only named exports. What about also marking the module as side-effect-free with sideEffects: false in package.json? Wouldn't that also help (when using bundlers like Webpack that support it)?

@stefcameron
Copy link
Member

  1. /* #__PURE__ */ comment before a call here

I don't understand this one. Because of the .join() call?

  1. tools don't want to remove top-level getters because of the potential side-effects and, unfortunately, there is no perfect way to make those treeshakeable. There are only quirk solutions for it - so it depends if you care about having this tree-shakeable. If yes, then I could give some guidance on how to fix this statement.

I'm not seeing how matches could be removed. It seems to be used by all of tabbable, isTabbable, and isFocusable. What am I missing?

Copy link
Member

@stefcameron stefcameron left a comment

Choose a reason for hiding this comment

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

This all looks good WRT changing the public exports shape!

@Andarist
Copy link
Contributor Author

This is looking good, thanks for updating to only named exports. What about also marking the module as side-effect-free with sideEffects: false in package.json? Wouldn't that also help (when using bundlers like Webpack that support it)?

Only to some extent - this only helps for ignoring imported files and as you only distribute a single file, it doesn't bring that much value. I would have to recheck but maybe it would help in such a case:

// foo.js
import { tabbable } from 'tabbable'

export function A() {
  tabbable()
}

export function B() {
  console.log()
}

// entry.js
import { B } from './foo'
console.log(B)

but it wouldn't help cases like this:

// entry.js
import { isTabbable } from 'tabbable'
console.log(isTabbable)

If anything gets required from tababble then its single file gets included and everything in it as well. Later this goes through tree-shaking algorithm (I don't treat "sideEffects" as part of this) and unused stuff might get removed, or not. It depends on how the code is written and if the optimizer is able to assess that unused stuff don't create any side-effects that could alter the behavior of the program if they would have been removed.

I don't understand this one. Because of the .join() call?

Yes. Tools don't know what .join does and it could potentially even make an important request so they have to bail out from removing this call and leave this expression in the consuming bundles. This comment is a hint for a minifier to drop it if the resulting value stays unused.

I'm not seeing how matches could be removed. It seems to be used by all of tabbable, isTabbable, and isFocusable. What am I missing?

Like in the first scenario that I have included in this post. It's about transitive dependencies. Tree-shaking (in my opinion) is not purely about scenarios when your library gets actually used but also about scenarios when it stays unused. Like when a lib library depends on tabbable, but the user only imports from lib stuff that does not depend on tabbable. Ideally, no tabbable code should get included in the final bundle.

@stefcameron
Copy link
Member

@Andarist Thanks for taking the time to answer my 3 questions, that's very helpful. This will be good to help with #39.

I do think that it would be worth adding sideEffects: false. I've seen it work well in certain scenarios where I've dealt with a library that was getting pulled in simply because of Webpack not knowing it was safe to exclude when nothing was using it anymore (i.e. the example you gave).

Might as well add the PURE comment also, as you suggested earlier.

@stefcameron stefcameron merged commit fc2689b into focus-trap:master Aug 25, 2020
stefcameron added a commit that referenced this pull request Aug 25, 2020
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.

Make this package tree-shakeable
2 participants