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

Minified files in archive #178

Merged
merged 3 commits into from May 24, 2020
Merged

Minified files in archive #178

merged 3 commits into from May 24, 2020

Conversation

vitkarpov
Copy link
Collaborator

It turns out there are no minified files inside distributives.

@vitkarpov vitkarpov requested a review from NikolayRys May 5, 2020 14:01
@vitkarpov vitkarpov added the Bug label May 5, 2020
@NikolayRys
Copy link
Owner

NikolayRys commented May 5, 2020

Great observation. But there is a little bit more to it.
In agreement with the Ilya Birman, the packaged version needs to be just two files: likely.css and likely.js.
So we need to amend the script itself to take only the two minified files and rename them from (likely.min.css -> likely.css).

Copy link
Owner

@NikolayRys NikolayRys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my comment above.

@vitkarpov
Copy link
Collaborator Author

Makes sense, will do!

@vitkarpov
Copy link
Collaborator Author

I'd suggest change the build in a way that likely.* would be minified. Instead of having *.min files, we'd have *.dev files unmodified. @NikolayRys what do you think?

@NikolayRys
Copy link
Owner

NikolayRys commented May 5, 2020

I have considered it but this feels a bit unusual - people won't be expecting it, and immediately be able to tell what's going on, without manually open the build files.
Because ".dev" kinda implies more than "min" does. It's not unreasonable to assume that
"dev", for example, may have more console logging or some other instrumentation.


but maybe I have a skewed perspective as a back-end developer :)

@vitkarpov
Copy link
Collaborator Author

I have considered it but this feels a bit unusual - people won't be expecting it, and immediately be able to tell what's going on, without manually open the build files.
Because ".dev" kinda implies more than "min" does. It's not unreasonable to assume that
"dev", for example, may have more console logging or some other instrumentation.

but maybe I have a skewed perspective as a back-end developer :)

Not at all, actually I have the same feeling. From the other perspective changing the meaning here feels like backward compatibility, without explicit renaming them before zipping in. Anyway, just wanted to talk about it 😄

@NikolayRys
Copy link
Owner

Not sure this will work 🤔
mv release/likely.min.js release/likely.js will probably fail, because this file already exists. Have you tried it in your console?

@vitkarpov
Copy link
Collaborator Author

vitkarpov commented May 5, 2020

Yep, I checked it works. Probably you're confusing with moving folders when it says "directory not empty".

@NikolayRys
Copy link
Owner

Sorry, but I'm not yet sure it's the way to do it. Consider this:
It deletes and rewrites the files in the current release. We'll need to run git reset each time after such packing. I suggest to do something like this:

mkdir release/zip
cp release/likely.min.css release/zip/likely.css
cp release/likely.min.js release/zip/likely.js
cross-env-shell bestzip release/$npm_package_name-$npm_package_version.zip release/zip/likely.css release/zip/likely.js
rm -rf release/zip

Copy link
Owner

@NikolayRys NikolayRys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^

@vitkarpov
Copy link
Collaborator Author

@NikolayRys Excuse me for the delay, fixed! Pls, check it out once again, thanks.

@vitkarpov vitkarpov requested a review from NikolayRys May 14, 2020 15:00
@NikolayRys NikolayRys added 3.0 Roadmap for the next version and removed 3.0 Roadmap for the next version labels May 24, 2020
@NikolayRys NikolayRys mentioned this pull request May 24, 2020
13 tasks
Copy link
Owner

@NikolayRys NikolayRys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks awesome, thanks for a good job

@NikolayRys NikolayRys merged commit e3d9d25 into master May 24, 2020
@NikolayRys NikolayRys deleted the add-min-files-to-archive branch May 24, 2020 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants