Skip to content

feat: add support for Babel macros #268

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

Merged
merged 4 commits into from
Jun 11, 2019

Conversation

FezVrasta
Copy link
Contributor

@FezVrasta FezVrasta commented Dec 10, 2018

I think it'd be very useful to support Babel macros out of the box, they allow to use Babel without additional configuration so they can improve a lot the developer experience without adding extra configuration to Microbundle.

Zero config tools such as create-react-app already support it.

@FezVrasta FezVrasta force-pushed the babel-macros branch 3 times, most recently from 0484b8f to 7e4baa5 Compare December 10, 2018 17:27
@Andarist
Copy link
Collaborator

Personally I would wait on #263 before merging this to keep more natural evolution of the babel support in microbundle. OTOH some babel support is already merged in (async functions~) so there is no much harm in this one.

@FezVrasta
Copy link
Contributor Author

@Andarist can I do something to speed up the release cycle? At my company we recently started using microbundle for a big open source effort we are conducting that will lead to the public release of a lot of npm packages. We'd like to help as we can to take advantage of this library.

Thanks!

@Andarist
Copy link
Collaborator

You can investigate why this fails - #263 (comment) . Or you can help @esphen implement this - rollup/rollup-plugin-babel#275 (he has already started some work)

@eliihen
Copy link

eliihen commented Dec 10, 2018

@FezVrasta Thanks for offering to help out!

I just dug into why the test on #263 fails, so no need to help out there anymore. Feel free to help out with rollup-plugin-babel if you want, though 🙂

This is a great suggestion, so we should definetely look into this once #263 is merged!

FezVrasta pushed a commit to quid/refraction that referenced this pull request Dec 11, 2018
this new fork includes:

- developit/microbundle#271
- developit/microbundle#268
- developit/microbundle#265

these changes are planned to be merged upstream anytime soon
@developit
Copy link
Owner

This seems useful enough to bring in.

@Andarist
Copy link
Collaborator

Andarist commented Jan 4, 2019

This would require adding a test before merging.

@FezVrasta
Copy link
Contributor Author

FezVrasta commented Jan 4, 2019

may someone help me understand why the snapshots test are failing? The only difference seem the letters used to minify the variable names

edit:
nevermind, I see it's failing even on master

@FezVrasta
Copy link
Contributor Author

@Andarist I added a test for the macro plugin

@@ -0,0 +1,20 @@
const { createMacro } = require('babel-plugin-macros');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this macro simply takes:

const name = macro();

and converts it to:

const name = 'name-macro';

@FezVrasta
Copy link
Contributor Author

I'm using this branch on my project and I noticed that if I reference a macro, it gets bundled in the dist file even if it should be treated as external.

Do you guys have any idea on why this could be happening?

Basically, if I use import styled from '@emotion/styled/macro', @emotion/styled will get embedded in the bundle.

@FezVrasta
Copy link
Contributor Author

Ok so, the issue was that the styled macro replaces its own import with @emotion/styled-base, and since that package isn't a dependency, it bundled it.

Adding it to the dependencies list is a "workaround", I'm not sure if there's any way to make this work in all the cases automatically tho.

FezVrasta pushed a commit to quid/refraction that referenced this pull request Jan 31, 2019
this new fork includes:

- developit/microbundle#271
- developit/microbundle#268
- developit/microbundle#265

these changes are planned to be merged upstream anytime soon
@FezVrasta
Copy link
Contributor Author

@Andarist could we merge this now that Microbundle makes use of Babel internally?

@ForsakenHarmony ForsakenHarmony changed the base branch from master to dev June 3, 2019 11:03
@ForsakenHarmony
Copy link
Collaborator

If you rebase once more 😇 I can merge it into dev which will be released soon (currently pre release)

@FezVrasta
Copy link
Contributor Author

Should I edit /lib/babel-custom.js?

@ForsakenHarmony
Copy link
Collaborator

Probably 🙃

@FezVrasta FezVrasta force-pushed the babel-macros branch 2 times, most recently from e8cf8b3 to 5f471fb Compare June 10, 2019 14:20
@FezVrasta
Copy link
Contributor Author

I rebased on top of dev but I now get this error in the new test this PR adds:

  ● fixtures › build macro with microbundle

    Cannot find module './macro' from '/Users/FezVrasta/Projects/microbundle/test/fixtures'

The import looks right, I'm not sure what's wrong

@ForsakenHarmony
Copy link
Collaborator

Weird issue, seems to be a problem with babel plugin macros or something 🤔

or rollup babel

@ForsakenHarmony
Copy link
Collaborator

nvm, our babel thing was broke

@ForsakenHarmony
Copy link
Collaborator

oh, now everything else is broken 👌

@ForsakenHarmony
Copy link
Collaborator

🎉🎉

@ForsakenHarmony ForsakenHarmony merged commit 905f62e into developit:dev Jun 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants