[WIP] Introduce build step to reduce CLI size #217

crutchcorn · 2020-05-18T05:45:46Z

The idea behind this PR came from #206. This PR is a WIP migration of that same idea without yarn or hotpatches

crutchcorn · 2020-05-18T05:46:25Z

@cspotcode I currently get the following error when trying to run plop on a plopfile.js:



TypeError: Cannot convert undefined or null to object
    at Function.keys (<anonymous>)
    at Object.e.prepare (/Users/crutchcorn/git/Plop/plop/dist/plop.js:543:7363)
    at t.exports (/Users/crutchcorn/git/Plop/plop/dist/plop.js:543:7046)
    at _.buildEnvironment (/Users/crutchcorn/git/Plop/plop/dist/plop.js:197:9835)
    at _.<anonymous> (/Users/crutchcorn/git/Plop/plop/dist/plop.js:197:10643)
    at /Users/crutchcorn/git/Plop/plop/dist/plop.js:197:10289
    at /Users/crutchcorn/git/Plop/plop/dist/plop.js:543:10553
    at /Users/crutchcorn/git/Plop/plop/dist/plop.js:577:1073

Any ideas on how this might be solved?

cspotcode · 2020-05-18T05:50:09Z

@crutchcorn not sure since I don't know any of the context for that error. If you run webpack with minification turned off, then the emitted file should be readable, and you can figure out the context.

crutchcorn · 2020-05-18T05:53:54Z

Hmm. Good call. The error was created with:

yarn build
cd examples
 node ../bin/plop.js

Without minification, the error is as such:

↳ node ../bin/plop.js
(node:42014) [DEP0005] DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead.
/Users/crutchcorn/git/Plop/plop/dist/plop.js:69963
  if (Object.keys(__webpack_require__(579).extensions).indexOf(ext) !== -1) {
             ^

TypeError: Cannot convert undefined or null to object
    at Function.keys (<anonymous>)
    at Object.module.exports.exports.prepare (/Users/crutchcorn/git/Plop/plop/dist/plop.js:69963:14)
    at module.exports.module.exports (/Users/crutchcorn/git/Plop/plop/dist/plop.js:69930:27)
    at Liftoff.module.exports.Liftoff.buildEnvironment (/Users/crutchcorn/git/Plop/plop/dist/plop.js:51017:3)
    at Liftoff.<anonymous> (/Users/crutchcorn/git/Plop/plop/dist/plop.js:51079:20)
    at /Users/crutchcorn/git/Plop/plop/dist/plop.js:51052:9
    at /Users/crutchcorn/git/Plop/plop/dist/plop.js:70245:14
    at /Users/crutchcorn/git/Plop/plop/dist/plop.js:106890:14

crutchcorn · 2020-05-18T05:59:37Z

This seems like it was worked around by @cspotcode by hotpatching rechoir:

https://github.com/cspotcode/plop/blob/de9f08b37c15d001e2414fe765740aa955db895b/patches/rechoir

Maybe you can provide some insight to what this is and how we might be able to unblock ourselves without hotpatching? Is this a bug upstream?

EDIT:

This isn't a bug upstream. This is simply a hotpatch to get rechoir working with webpack's require properly. I'll see a way if we can contribute this upstream or what we can otherwise do about this

cspotcode · 2020-05-18T06:09:30Z

Oh yeah, I remember that. Rechoir needs to do certain require() calls at runtime, meaning webpack is not allowed to analyze it and try to bundle the target of the require() call.

Webpack has a mechanism called __non_webpack_require__ for these situations. It compiles down to require, bypassing webpack's bundler.
https://webpack.js.org/api/module-variables/#__non_webpack_require__-webpack-specific-

The tricky part is that most require calls in that file should be analyzed and bundled. So you can't completely exclude the file from parsing via module.noParse.

IgnorePlugin might be helpful here but I'm not sure.
https://webpack.js.org/plugins/ignore-plugin/

You could also send a PR to rechoir, but I'm not 100% sure what that PR should look like.

crutchcorn · 2020-05-18T06:29:12Z

I tried leaving in IgnorePlugin, but it seems like it's not meant for what we're trying to do:

		new webpack.IgnorePlugin({
			resourceRegExp: /\./,
			contextRegExp: /rechoir/
		}),

Simply generates:

  if (Object.keys(!(function webpackMissingModule() { var e = new Error("Cannot find module 'undefined'"); e.code = 'MODULE_NOT_FOUND'; throw e; }()).extensions).indexOf(ext) !== -1) {
    return true;
  }

By design: webpack/webpack#2858

I think resolve.alias might do what we're trying to do:

https://webpack.js.org/configuration/resolve/

I'll take another stab at hotpatching this tomorrow

…or extensions, clean up webpack config

chore: add require fallback to webpack require, use default require for extensions, clean up webpack config

crutchcorn · 2021-05-19T01:31:59Z

I may have initially opened this PR but all of the heavy lifting of getting this finally working goes to @evelynhathaway. She's the one that got this working finally in a place where I feel comfortable with the pros and cons

Here's where we're at:

Plop, after this PR, will be 1.2MB distributed. From 48MB, that's a shrinkage of 40x.

This PR does not require yarn or patching node-plop or rechoir or any other NPM package based on diffing.

Instead, this new method relies on exposed Webpack hooks that allow us to modify the behavior of Webpack's require logic. This is done via a Webpack plugin that @evelynhathaway worked hard on producing for us.

There are some minor downsides, however:

We may run into a scenario where Webpack's hooks are changed during a release. @evelynhathaway was gracious enough to walk through the source code with me in a pair-coding session, and based on the code she highlighted, this seems like it would be unlikely until a major release of webpack. However, even if this does occur before then, the relevant code of where to look has been saved to code comments. This would make debugging a potential break much easier than it was to get it spun up in the first place, since we have a starting point to look at
Because we're changing the behavior of Webpack's require, we may run into issues with code such as require.resolve, which is used in a PR we're interested in moving forward with soon (NPM-Based Reusable Generators #248). That said, we don't think this should break due to the method of implementation. If it does, @evelynhathaway has mentioned that she'd be happy to revisit and assist us in updating the Webpack plugin. She has a fantastic history of OSS contributions and am confident in her ability to resolve this if it ever comes up

That said, let's look at what we're planning for next steps.

Next Steps

This PR will be a "breaking change". Nothing in the API should change, but we previously weren't documenting how to handle deps such as inquirer-directory or plop-pack-fancy-comments. As such, it's not technically a breaking change, assuming you weren't implicitly relying on those packages being installed, but I'd rather be safe than sorry.

We're also acutely aware of how drastic of a change this is and how subtly things could (theoretically) break without being immediately noticed. As such, while this PR is fully functional as far as my testing has shown, we will not be merging this in immediately.

Instead, @evelynhathaway has mentioned that she'd be willing to help us get e2e testing in our CLI repo. We plan to complete that work and merge that before merging this PR. This should allow us to be more confident in bigger changes moving forward.

Webpack Plugin

After thinking through this a bit further (both on my own and in discussions with @evelynhathaway ), I think we should make this Webpack plugin available as a separate NPM package. The plugin allows Webpack to bundle static assets, but still allow dynamic resolution of require items. This seems like a valuable plugin for other projects.

However, because of it's tight coupling to Plop, we'd like to keep this plugin in the plopjs org (which @evelynhathaway agrees would be okay with her)

Making this plugin a dedicated package would allow us to:

Add testing for the plugin to ensure it's doing what it should
Add longer-form documentation so that new contributors are not confused of it's purpose
Keep the PlopJS repo clean and focused

We're thinking that the dedicated plugin package should be done after merging this PR in (which is after e2e tests).

@amwmedia, do you have any thoughts on this? Any opposition on potentially adding @evelynhathaway to the PlopJS org in the future to allow this?

amwmedia · 2021-05-20T13:31:36Z

First of all, thank you both for all of the time and effort you have devoted to pushing Plop forward. The project has come a long way from where I started it and all of the recent progress has only been possible due to the passion and efforts of the community.

The plan you two have put together sounds solid to me. I've already added @evelynhathaway to the plop org and I'm very happy to welcome her to the team!

cspotcode · 2021-05-24T20:28:36Z

I wanted to try this out, but it doesn't seem to be working for me.

I created a reproduction which runs on github actions. Is there anything I'm doing wrong?
https://github.com/cspotcode/repros/tree/plop-webpack
https://github.com/cspotcode/repros/actions/runs/872603595

evelynhathaway · 2021-05-24T20:44:43Z

After a quick test, I can reproduce something similar. It seems to be a MODULE_NOT_FOUND error. It sucks that any error would dump MBs of text into your console for a stack trace 😔

I'll look into it later today.

cspotcode · 2021-05-24T20:47:29Z

Yeah, plus it's odd that it errors at all, since the plopfile does exist.

evelynhathaway · 2021-05-24T23:24:43Z

The latest commit works in development mode and handles edge cases we don't even run into, but relies on the function names not being minified. We can either not minify the function names, or I can try something else.

evelynhathaway · 2021-05-25T00:36:57Z

@cspotcode can you rerun your reproduction action? It should be fixed now.

cspotcode · 2021-05-25T04:54:21Z

Yeah, looks good. https://github.com/cspotcode/repros/runs/2661854016?check_suite_focus=true

…

On Mon, May 24, 2021, 8:37 PM Evelyn Hathaway ***@***.***> wrote: @cspotcode <https://github.com/cspotcode> can you rerun your reproduction action? It should be fixed now. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#217 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAC35OACIOC555RDMSQB65DTPLWLPANCNFSM4NDYHVGQ> .

crutchcorn · 2022-04-26T00:13:10Z

After some discussion with @evelynhathaway, we've decided that we're not moving forward with this idea.

It adds a non-trivial amount of code complexity (especially for monorepo setups) for (what we perceive to be) minimal rewards.

Initial work to introduce webpack

8666750

crutchcorn mentioned this pull request May 18, 2020

Webpack to reduce install size from 48MB to about 6MB #206

Closed

chore: add require fallback to webpack require, use default require f…

9d1aaa4

…or extensions, clean up webpack config

evelynhathaway mentioned this pull request May 16, 2021

chore: add require fallback to webpack require, use default require for extensions, clean up webpack config #266

Merged

evelynhathaway and others added 2 commits May 18, 2021 17:57

fix: export a commonjs module, add code comments

aab1ef0

Merge pull request #266 from evelynhathaway/webpack

5145e8f

chore: add require fallback to webpack require, use default require for extensions, clean up webpack config

fix: more thoroughly handle webpack require contexts

a59b8ed

evelynhathaway added 2 commits May 24, 2021 16:34

fix: keep webpack require context names

af49b9f

fix: replace module.exports in webpack cache

e3714fa

crutchcorn closed this Apr 26, 2022

crutchcorn deleted the webpack branch April 27, 2022 06:26

crutchcorn mentioned this pull request Sep 5, 2023

Plop 4 Planning #393

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Introduce build step to reduce CLI size #217

[WIP] Introduce build step to reduce CLI size #217

crutchcorn commented May 18, 2020 •

edited

Loading

crutchcorn commented May 18, 2020

cspotcode commented May 18, 2020

crutchcorn commented May 18, 2020

crutchcorn commented May 18, 2020 •

edited

Loading

cspotcode commented May 18, 2020

crutchcorn commented May 18, 2020

crutchcorn commented May 19, 2021

amwmedia commented May 20, 2021

cspotcode commented May 24, 2021 •

edited

Loading

evelynhathaway commented May 24, 2021

cspotcode commented May 24, 2021 •

edited

Loading

evelynhathaway commented May 24, 2021

evelynhathaway commented May 25, 2021

cspotcode commented May 25, 2021 via email

crutchcorn commented Apr 26, 2022

[WIP] Introduce build step to reduce CLI size #217

[WIP] Introduce build step to reduce CLI size #217

Conversation

crutchcorn commented May 18, 2020 • edited Loading

crutchcorn commented May 18, 2020

cspotcode commented May 18, 2020

crutchcorn commented May 18, 2020

crutchcorn commented May 18, 2020 • edited Loading

cspotcode commented May 18, 2020

crutchcorn commented May 18, 2020

crutchcorn commented May 19, 2021

Next Steps

Webpack Plugin

amwmedia commented May 20, 2021

cspotcode commented May 24, 2021 • edited Loading

evelynhathaway commented May 24, 2021

cspotcode commented May 24, 2021 • edited Loading

evelynhathaway commented May 24, 2021

evelynhathaway commented May 25, 2021

cspotcode commented May 25, 2021 via email

crutchcorn commented Apr 26, 2022

crutchcorn commented May 18, 2020 •

edited

Loading

crutchcorn commented May 18, 2020 •

edited

Loading

cspotcode commented May 24, 2021 •

edited

Loading

cspotcode commented May 24, 2021 •

edited

Loading