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

Setup rollup build, add TS types #38

Closed
wants to merge 1 commit into from

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented Mar 5, 2019

I'd actually want to tweak build in https://github.com/davidtheclark/focus-trap-react but thought I could work on the whole thing bottom-up and that's how I ended up doing this.

If you like it I will prepare followup PRs in related packages.

⚠️ This is a breaking change due to exports shape change. The proposed form makes this library way more tree-shakeable (static properties are treeshaking/DCE enemies)

@davidtheclark
Copy link
Collaborator

Hi @Andarist. I'm not interested in overhauling the codebase this dramatically. Is there a problem you're aiming to solve here?

@Andarist
Copy link
Contributor Author

Andarist commented Mar 9, 2019

Basically, I wanted to optimize https://github.com/davidtheclark/focus-trap-react (bundle size), but decided to work on this bottom-up. This PR makes this lib more bundler friendly, more future proof (ESM modules, CJS only as fallback). This also provides TS typings.

I understand this might seem like a lot but in its essence, this just split a single file into multiple to improve readability and changes exports shape - it exports things separately instead of attaching utils as static properties of the main export.

If you are going to vote against this then I would be ok with it, no harsh feelings - I've decided to work on this knowing that this might get rejected. You can tell me which parts do you like - i.e. updating deps, eslint, using babel, whatever and I can remove unwanted parts to get some of the things merged in.

That being said - I strongly think that this improves the package overall, maybe not dramatically, but every bit counts 😉

@davidtheclark
Copy link
Collaborator

@Andarist If you'd like to contribute a change, please open an issue describing what you'd like to do, and please open separate issues for separate changes (e.g. an issue for using Babel, an issue for TS typings). This would help focus any time spent on discussing the changes.

Non-breaking version upgrades to dependencies are always welcome as PRs.

@brian-lagerman
Copy link

I put in a PR for ES6 Support on focus-trap focus-trap/focus-trap#84 without having seen this. However, this would further lighten the load as tabbable wouldn't need to be bundled anymore from focus-trap's perspective. Is there any progress on the concerns with this PR?

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.

3 participants