NikolayRys / Likely

The social sharing buttons that aren’t shabby
ISC License
394 stars 61 forks source link

Keep unminified JS files in builds #165

Closed vitkarpov closed 4 years ago

vitkarpov commented 4 years ago

Fixes #149

This is a breaking change! (for 3.x release)

πŸ‘† that's how build folder looks like after running npm run-script build. What's been likely before turns into likely.min now and likely file is unminified instead.

How does it work

Minification feature is built-in and runs under -p flag. There are 2 configs now: base is what we already have and webpack.config.min.js inherits the base one. The min config has "min-postfixed keys" in entries to get [name].min.js files.

npm run-script build runs Webpack twice: one without -p mode with the base config and another in production mode (that's where minifications come) with the min config (to get proper file names).

vitkarpov commented 4 years ago

@NikolayRys I'm not sure why tests fail πŸ€” Is it something CI specific? I didn't notice that locally, will take a closer look then.

NikolayRys commented 4 years ago

Hi, thank you for the pull request, we need it for 3.0.

I'm a bit busy right now, but I'll review this one and other PRs next week.

NikolayRys commented 4 years ago

@vitkarpov, awesome job, thank you!

vitkarpov commented 4 years ago

@NikolayRys done πŸ‘Œ

vitkarpov commented 4 years ago

@NikolayRys I reviewed this PR one again and noticed a thing I'm not sure about and one thing that probably needs to be fixed:

vitkarpov commented 4 years ago

there are 2 files: likely.js & likely-common.js but actually they are the same since it's Webpack universal module definition, so a single file works both in the browser and node.js, what do you think? (please, look at built files)

Nah, I see the difference now:

window.addEventListener('load', () => {
    likely.initiate();
});

Everything's fine then.

NikolayRys commented 4 years ago

Oh, it's a very good point about css. I think, we need to be consistent now and have likely.min.css minified as well, could you please add it too?

Also, please remove the 3.0.0 bump, we are not there yet to finalize the release, and there will be several more changes to it, here's the list: #163 It's okay if this change brakes it because it happens only in the master, which is not meant to be stable.

vitkarpov commented 4 years ago

I think, we need to be consistent now and have likely.min.css minified as well, could you please add it too?

Yes, will do within this PR.

Also, please remove the 3.0.0 bump, we are not there yet to finalize the release

Gotcha, do we need to add built files to the repo then? Anyway, it shouldn't hurt anybody.

vitkarpov commented 4 years ago

@NikolayRys Done πŸŽ‰

NikolayRys commented 4 years ago

Thanks for the good work, this was a useful change.

About the builds, we were keeping them always in this project in the repo, it's just for consistency and for the case if somebody wants the edge version :)