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

[New] Manually add timbre w/ npm auto-update #6295

Closed
wants to merge 3 commits into from

Conversation

maruilian11
Copy link
Contributor

git repo url: https://github.com/mohayonao/timbre.js
Watch 59
Star 611
Fork 50

@Piicksarn could u help me check this PR for #6110 ? thank you!

  • Not found on cdnjs repo
  • No already exist issue and PR
  • Notable popularity
  • Project has public repository on famous online hosting platform (or been hosted on npm)
  • Has valid tags for each versions (for git auto-update)
  • Auto-update setup
  • Auto-update target/source is valid.
  • Auto-update filemap is correct

@Piicksarn
Copy link
Contributor

@PeterDaveHello, I think the pr is fine!
would you mind to have a look at the pr?
Thank you!

@maruilian11
Copy link
Contributor Author

@Piicksarn i have rebased it, could you help me check it again?

@maruilian11
Copy link
Contributor Author

@Amomo could you help me check this PR? thank you!

@Amomo
Copy link
Contributor

Amomo commented Nov 30, 2015

@maruilian11
According to its README.md, I think "timbre.node.js" is a node module.
We should add "timbre.dev.js", not "timbre.node.js".

"timbre.node*.js"
]
},
"requiredFiles": [
Copy link
Contributor

Choose a reason for hiding this comment

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

You have point the minified file in "filename" field, no need to add "requiredFiles".

@maruilian11
Copy link
Contributor Author

@Amomo thank you~ i have modified it~

@Amomo
Copy link
Contributor

Amomo commented Dec 21, 2015

@PeterDaveHello I think this PR is fine, could you take a look? Thank you! :-)

@Amomo Amomo assigned PeterDaveHello and unassigned Amomo Dec 21, 2015
@PeterDaveHello
Copy link
Contributor

Use dev file as main file is strange, any other source we can use to import or download the files?

@PeterDaveHello
Copy link
Contributor

Any updates here? Thanks!

@maruilian11
Copy link
Contributor Author

@PeterDaveHello ping the author in the issue.
mohayonao/timbre.js#57 (comment)

@PeterBot
Copy link
Contributor

PeterBot commented Apr 4, 2017

Any updates here? Thanks!

1 similar comment
@PeterBot
Copy link
Contributor

Any updates here? Thanks!

@maruilian11
Copy link
Contributor Author

@PeterDaveHello I use Gruntfile.js to generate timbre.js and timbre.js.map, and find timbre.js source is timbre.dev.js. Since the author has no replied for a long time, can we directly add timbre.js and timbre.dev.js in this case?

@PeterDaveHello
Copy link
Contributor

We can manually build and add it from tagged commit in this situation.

@PeterDaveHello
Copy link
Contributor

PeterDaveHello commented Sep 17, 2017

That's funny ... isn't this simply working? uglifyjs --source-map -o timbre.js timbre.dev.js

I think it clearly told you the problem, didn't it? ERROR: './timbre.js.map' is not a supported option

So you both stock on the problem that you don't take a clear look at the error message ...

Take a look at the help message and I didn't see any of them telling to specify the filename of map file (so where is option --unsafe come from? The earlier version?):


peter $ uglifyjs -V
uglify-js 3.1.1

peter $ uglifyjs --help

  Usage: uglifyjs [options] [files...]


  Options:

    -V, --version                output the version number
    -p, --parse <options>        Specify parser options.
    -c, --compress [options]     Enable compressor/specify compressor options.
    -m, --mangle [options]       Mangle names/specify mangler options.
    --mangle-props [options]     Mangle properties/specify mangler options.
    -b, --beautify [options]     Beautify output/specify output options.
    -o, --output <file>          Output file (default STDOUT).
    --comments [filter]          Preserve copyright comments in the output.
    --config-file <file>         Read minify() options from JSON file.
    -d, --define <expr>[=value]  Global definitions.
    --ie8                        Support non-standard Internet Explorer 8.
    --keep-fnames                Do not mangle/drop function names. Useful for code relying on Function.prototype.name.
    --name-cache <file>          File to hold mangled name mappings.
    --self                       Build UglifyJS as a library (implies --wrap UglifyJS)
    --source-map [options]       Enable source map/specify source map options.
    --timings                    Display operations run time on STDERR.
    --toplevel                   Compress and/or mangle variables in toplevel scope.
    --verbose                    Print diagnostic messages.
    --warn                       Print warning messages.
    --wrap <name>                Embed everything as a function with “exports” corresponding to “name” globally.
    -h, --help                   output usage information

The options should be used IMO should be --compress, --source-map & --mangle, remove the source map option when you don't need it.

@PeterDaveHello PeterDaveHello dismissed stale reviews from sufuf3 and dakshshah96 September 17, 2017 13:32

WIP

Copy link
Contributor

@PeterDaveHello PeterDaveHello left a comment

Choose a reason for hiding this comment

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

Anyway, I didn't see a need to use Makefile to make the non-minified dist file in version 13.02.06 as it's already there, isn't it? @maruilian11 ?

So what all you may need to do is to minify some versions?

Please clearly mention the details in the commit message and comment so that reviewer can understand what's really going on here.

@maruilian11
Copy link
Contributor Author

@PeterDaveHello I think @sufuf3 looked the error msg and find the problem. So you may misunstand her.

@maruilian11
Copy link
Contributor Author

Anyway, I didn't see a need to use Makefile to make the non-minified dist file in version 13.02.06 as it's already there, isn't it? @maruilian11 ?

@PeterDaveHello Sorry, I don't get your meaning. I didn't use Makefile to make the non-minified dist file, but to produce a official file of it.

@PeterDaveHello
Copy link
Contributor

@maruilian11 The file is already there, isn't it?

@maruilian11
Copy link
Contributor Author

@PeterDaveHello Yes, timbre.dev.js is already there, but timbre.js and timbre.js.map need to be produced.

@PeterDaveHello
Copy link
Contributor

So you don't need to deal with Makefile, do you?

To summarize the situation here, this PR is waiting for some versions need to be added manually and those versions need manually minification also? If so, can we simply use git ignore them to make the case simple?

@maruilian11
Copy link
Contributor Author

@PeterDaveHello I don't get it. You mean we minify ourself? Not through the Makefile?

@PeterDaveHello
Copy link
Contributor

@maruilian11 Does Makefile contain the minification part? I don't think so ...

@maruilian11
Copy link
Contributor Author

@PeterDaveHello Yes, it does.
Following content is from timbre@13.1.18 Makefile in npm package:

gh-pages: clear
@UglifyJS --unsafe -nc -nm -o ./timbre.js ./timbre.dev.js

@PeterDaveHello
Copy link
Contributor

Okay, fine, it's part of gh-pages which is not a common case.

@PeterDaveHello
Copy link
Contributor

So let's finish this PR ASAP? Just add the few missing versions.

`timbre.js` and `timbre.js.map` are built from Gruntfile.js.
close cdnjs#6110, cc @mohayonao
Manually add them from npm package because `timbre.js` and
`timbre.js.map` need to be built manually.
Manually add them from git repo because `timbre.js` needs to be built
manually.

v13.01.20:
mohayonao/timbre.js@0becae5

v13.02.02:
mohayonao/timbre.js@b24652e

v13.02.06:
mohayonao/timbre.js@afb9507

v13.04.19:
mohayonao/timbre.js@698bd23

Others are added accord to git tag.

cc cdnjs#6110
@maruilian11
Copy link
Contributor Author

@PeterDaveHello The test seems not working. Please take a look.

@sufuf3 sufuf3 requested review from PeterDaveHello, sufuf3 and extend1994 and removed request for pvnr0082t December 27, 2017 08:51
@PeterBot
Copy link
Contributor

Any updates here? Please let me know if you need any help. Thanks!

(This is an automatic ping message, sorry for disturbing, we will get back here ASAP.)

cc @sashberd

@sashberd
Copy link
Contributor

I have tried to rebase and push tis commit in order to send it again to check. The rebase was succeed, but push provides me next message:
"To https://github.com/maruilian11/cdnjs.git
! [remote rejected] timbre -> timbre (permission denied)
error: failed to push some refs to 'https://github.com/maruilian11/cdnjs.git'
"

@extend1994 @sufuf3 @PeterDaveHello

@sashberd
Copy link
Contributor

@PeterDaveHello Is this PR only need to be rebased and then merged?

Copy link
Member

@xtuc xtuc left a comment

Choose a reason for hiding this comment

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

Remove the versions and only commit ajax/libs/timbre/package.json

@sashberd
Copy link
Contributor

Moved to #13700

@sashberd sashberd closed this Feb 25, 2020
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

10 participants