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

Replaced custom bash script for copy-mjs tool #1366

Closed
wants to merge 1 commit into from

Conversation

Andarist
Copy link

@Andarist Andarist commented Jun 4, 2018

I've packaged this script into a reusable tool to use in another project, thought you might like it here too. It's advantage is that it should run fine on windows too.

@IvanGoncharov
Copy link
Member

@Andarist Thanks for PR 👍
It always great to replace hackish shell script with a proper solution.
But I think it's better to follow the rest of JS ecosystem and switch *.mjs files.
Also Babel added --keep-file-extension that allows us to achive the same result without the need for additional scripts: babel/babel#6221

@mjmahone
Copy link
Contributor

mjmahone commented Jun 7, 2018

I'm not 100% sure what @leebyron 's philosophy on pulling in 3rd-party scripting tools is, and whether it's OK to add this dev dependency. My gut says it likely is OK, but I probably will wait until after the 14.0 release to fully consider this PR: it won't be a breaking change regardless, and I want to reduce the moving parts to just code/node-version changes as much as possible.

But thank you @Andarist for contributing to cleaning up hackish shell scripts!

@IvanGoncharov
Copy link
Member

@Andarist @mjmahone I like how it's done in react:

https://github.com/facebook/react/blob/52fbe7612e0527b8c86decac519c344626f6bd72/package.json#L108
I would prefer to have everything in one resources/build.js file.
Plus all major processing is done by babel in parallel so build.js can use only sync functions like fs.*Sync and execSync.

@Andarist
Copy link
Author

Andarist commented Jun 7, 2018

I agree that react's script is really nicely written. It's usually just way more time consuming to setup it like this (initially).

I don't believe that using sync-only functions is the way to go, as requirements arise its more future-proof to use async from the start.

My copy-mjs package exports an async function for programatic invocation too, if u want to reuse it in such a script. I could take some time in the future to create such react-like build script, but as @mjmahone mentioned it may be good to start working on it after next minor release. Either way - ping me if you want some help with it.

@leebyron
Copy link
Contributor

While bash can be weird, I'd honestly prefer having fewer dev dependencies just so that we can more easily control this build process in the future, especially since this tool seems to only replace half of this build step instead of all of it. It's likely that we may want to use babel's --keep-file-extension in the future as a simplifying element - that would cause us to remove the use of this. I'd suggest that we leave things as is until we can holistically improve the build.

A js script that runs the whole build like React has set up may be easier to work on than our patched together npm and bash scripts.

I don't believe that using sync-only functions is the way to go, as requirements arise its more future-proof to use async from the start.

@Andarist I'd challenge you to question this assumption. I believe what you say is likely true if your intent is to support web servers or other highly parallel operations, but build scripts tend to be highly serial. I seem to remember joliss doing some deep experimentation on this point while building the broccoli build to and found that async added measurable overhead to build time in aggregate.

@Andarist
Copy link
Author

@Andarist I'd challenge you to question this assumption. I believe what you say is likely true if your intent is to support web servers or other highly parallel operations, but build scripts tend to be highly serial.

I agree it's highly serial - it doesn't mean that whole process has to be synchronous though. Certain steps can be async for perf-boost or just because the author thought it would be a good idea (author of certain step-util/tool that is)

@IvanGoncharov
Copy link
Member

Closing this PR since:

A js script that runs the whole build like React has set up may be easier to work on than our patched together npm and bash scripts.

is implemented in #1841

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.

None yet

5 participants