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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Draft] Full rewrite #2

Merged
merged 8 commits into from Jan 4, 2016
Merged

[Draft] Full rewrite #2

merged 8 commits into from Jan 4, 2016

Conversation

mattdesl
Copy link
Collaborator

@mattdesl mattdesl commented Jan 4, 2016

I was looking into doing this today, and I started writing the exact same module (down to the same name, hehe) so I figure I'd just fork & submit a PR instead of fragmenting npm unnecessarily. 馃槃

Here is a draft, not complete. Some things:

  • filename is now required as the input for the transpiler, since that's what browserify transforms expect
  • opt.transform now matches the way browserify resolves and handles transforms. Currently uses resolve.sync since the async API would cause a lot more code complexity.
  • added subarg handling for --transform in CLI
  • added alias -t in CLI
  • added a small test; note the devDependencies have their versions pinned to reduce the chance that patches will break the tests
  • basedir can be specified from API/CLI to resolve differently
  • added standard linting, npmignore, and some tweaks to readme.

Something to still figure out is the general user-experience of the CLI/API, especially for whole directories (like babel does). I'm not sure whether vinyl-fs or something similar could help clean the code up.

Directories:

# Option A: input is a directory, like babel does
transpilify src -t brfs --out-dir dist/

# Option B: input is globs, output is resolved based on cwd
transpilify src/**/*.js -t brfs --out-dir dist/

Single-file:

# input is a single file, user wants to specify output file instead of piping
# same flag as babel
transpilify src/index.js -t brfs --out-file dist/index.js

# this will throw an error, expected --out-dir
transpilify src/**.js -t brfs --out-file dist/index.js

/cc @hughsk since this tool could be useful for glslify modules 馃槃

@ahdinosaur
Copy link
Owner

hey @mattdesl, this is really great, thanks for your help. :)

code looks good to me. i gave you write access to this repo and the npm module, feel free to merge and publish when you feel it's complete.

i think your ideas for the CLI/API are great, i had something similar in mind last night but just wanted to get a minimum transpiler using browserify transforms pushed so we could start iterating.

@ahdinosaur
Copy link
Owner

regarding CLI/API: do we want to merge the browserify config from the package.json local to filename into our options? my initial feeling is yes.

@mattdesl
Copy link
Collaborator Author

mattdesl commented Jan 4, 2016

I'm not sure if it's a good idea to merge package.json config, because:

  1. This isn't applying transforms to node_modules (dependencies) which is where the browserify config is really important.
  2. If you use transpilify on pre-publish then you don't need the package.json config "browserify" field on npm anymore. Having it there might lead to source being transformed twice (first by transpilify, then by browserify on bundle).

However something similar might be worth considering to allow users to setup config without long shell commands. 馃槃

Pushed vinyl-fs which seem to handle a lot of the file streaming/copying for us. It is not part of the programmatic API but it seems like neither is Babel's multi-output option.

@hughsk
Copy link
Collaborator

hughsk commented Jan 4, 2016

@mattdesl haha, I was thinking of suggesting we write up something like this earlier today :P Thanks for the heads up!

+1 on ignoring package.json transforms in this case, maybe transpilify.transform (or transforms) instead of browserify.transform might be a good place to put them instead? Would love it to become something as simple as what we have now for browserify transforms. I've tried setting up packages on npm that use babel but it always seems to add a lot of boilerplate and extra thought :(

@ahdinosaur
Copy link
Owner

馃憤 transpilify.transform.

also @hughsk gave you write access as well just in case. :)

@ahdinosaur ahdinosaur mentioned this pull request Jan 4, 2016
Closed
@mattdesl mattdesl merged commit 9c7c0a9 into ahdinosaur:master Jan 4, 2016
@mattdesl mattdesl deleted the rewrite branch January 4, 2016 15:39
@mattdesl
Copy link
Collaborator Author

mattdesl commented Jan 4, 2016

Ok, I am going to merge and continue iterating on master.

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.

None yet

3 participants