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

Keep unminified JS files in builds #165

Merged
merged 4 commits into from Feb 19, 2020
Merged

Keep unminified JS files in builds #165

merged 4 commits into from Feb 19, 2020

Conversation

vitkarpov
Copy link
Collaborator

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
Copy link
Collaborator Author

@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.

@vitkarpov vitkarpov self-assigned this Jan 10, 2020
@vitkarpov vitkarpov closed this Jan 10, 2020
@vitkarpov vitkarpov reopened this Jan 10, 2020
@NikolayRys
Copy link
Owner

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
Copy link
Owner

NikolayRys commented Feb 19, 2020

@vitkarpov, awesome job, thank you!

@NikolayRys NikolayRys mentioned this pull request Feb 19, 2020
14 tasks
@vitkarpov
Copy link
Collaborator Author

@NikolayRys done 馃憣

@vitkarpov
Copy link
Collaborator Author

@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:

  • 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)
  • likely.css becomes unminified. Do we need likely.min.css or leave likely.css minified?

@vitkarpov
Copy link
Collaborator Author

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
Copy link
Owner

NikolayRys commented Feb 19, 2020

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

@NikolayRys Done 馃帀

@NikolayRys
Copy link
Owner

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 :)

@NikolayRys NikolayRys merged commit c1565fe into master Feb 19, 2020
@NikolayRys NikolayRys deleted the issue-149 branch February 19, 2020 20:08
@NikolayRys NikolayRys mentioned this pull request Apr 29, 2020
13 tasks
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.

Use likely.min.js for the file name in release
2 participants