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

Move traverse logic to Program exit #27

Closed
wants to merge 1 commit into from

Conversation

tbranyen
Copy link

@tbranyen tbranyen commented Sep 27, 2019

I'm apologize for issuing this PR, because it's tailored to the use case
I have of running CommonJS modules in the browser. I've written a
plugin: https://github.com/tbranyen/babel-plugin-transform-commonjs
which converts CommonJS to browser-compatible ESM. I could only think of
two ways to use the two togther: 1) this change (or something similar to
it) is made, 2) we wait until Babel implements a plugin ordering
system. I believe they are working on the latter.

My CommonJS plugin must run after all other transforms so that dead code
elimination can work properly. What this effectively does is halt the
traversal till the Program exit consistently. This will work reliably
for every plugin that does this, and ensures that all transforms have
run (and may run again) prior to running this plugin.

Thanks in advance!

@tbranyen
Copy link
Author

tbranyen commented Sep 27, 2019

I'm seeing test failures locally and now seeing them in CI, so I'll work on fixing them.

Edit: should be fixed now.

I'm apologize for issuing this PR, because it's tailored to the use case
I have of running CommonJS modules in the browser. I've written a
plugin: https://github.com/tbranyen/babel-plugin-transform-commonjs
which converts CommonJS to browser-compatible ESM. I could only think of
two ways to use the two togther: 1) this change (or something similar to
it) is be made, 2) we wait until Babel implements a plugin ordering
system. I believe they are working on the latter.

My CommonJS plugin must run after all other transforms so that dead code
elimination can work properly. What this effectively does is halt the
traversal till the Program exit consistently. This will work reliably
for every plugin that does this, and ensures that all transforms have
run (and may run again) prior to running this plugin.

For now I'm going to publish a fork under my personal npm namespace so I
can keep moving, but I'd love to chat and figure out a way to either
land this or figure out a different solution.

Thanks in advance!
@coreyfarrell
Copy link
Member

I'm not sure how I feel about making this change for everyone. One thought I have is that this plugin could be wrapped:

'use strict';

const basePlugin = require('./index.js');

module.exports = babel => {
  const baseInstance = {...basePlugin(babel)};
  return {
    ...baseInstance,
    visitor: {
      Program: {
        exit(path, state) {
          path.traverse(baseInstance.visitor, state);
        }
      }
    }
  };
};

// Below is optional only if you need this function call.
module.exports.resolve = basePlugin.resolve;

If this script were created as a program-exit.js you could use babel-plugin-bare-import-rewrite/program-exit in your babelrc. This way you get any bug-fixes that come into this base plugin but without the risk of unintended consequences to other users (you need this plugin to be last, maybe someone else needs it to not be last). Also it is my understanding that path.traverse adds overhead that could be significant (admittedly I could be mistaken).

As an alternative you could update your fork of to simply extend this plugin as shown above (just alter the require statement to pull my module. This might not be a bad idea for now anyways, I've already merged a semver-major change but I want to deal with #23 before doing a new release.

@tbranyen
Copy link
Author

Okay, I'll explore a way of making this non-default behavior. There's no huge rush, but this plugin does everything correctly to load a reasonably complex situation so I'd like to get it working with the example app I linked in the issue. While my fork works, I'd like to see unified tooling with Babel, so getting rid of it is a priority for me.

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.

2 participants