search

Pomax / node-flickrapi

A node.js (and client-library) implementation of the Flickr API with oauth API key authentication and API method proxying
176 stars 51 forks source link

update browser api file support `import`, inspiration from jquery #86

Closed genffy closed 6 years ago

genffy commented 8 years ago

when i use this api in my personal project with webpack and found that browser/flickrapi.dev.js and browser/flickrapi.js not support es6 module loader.

the test files are app.js test.html webpack.js to run test just 

npm install 
cd browser && webpack --config webpack.js

then open test.html in browser

Pomax commented 8 years ago

heya, thanks for the PR - I've added some comments in the various files where I think some more work is necessary before this can be merged in

genffy commented 8 years ago

@Pomax thank you very much review that. Now i fixed some notes and update compile.js.

Pomax commented 8 years ago

nice, thanks - the updates to compile.js however are a bit dangerous: it looks like you also ran a blanket "fix indentation" operation on the file which is absolutely not what should have happened, because the spacing in that file is critical - large parts of the file defined what gets written to the library how, so by changing the indentation you also changed the content formatting of the compilation result.

If possible, please restore the compile.js file and only add the bits you need added using the same code style, and then either open a new issue (e.g. "fix the indentation and spacing for compile.js") with its own PR, or just leave it as is. Auto-style-fixing is not a good idea here.

Pomax commented 8 years ago

Thanks for updating the PR, this'll be useful to quite a few people! Can I get you to still please address https://github.com/Pomax/node-flickrapi/pull/86/files#r68060927, though? I'll try to get it merged in after that when I have some more free time.

Pomax commented 8 years ago

Thanks! Can you run the dev compile again too, so the spacing in the compiled lib is fixed too? (I know, I'm sorry for asking)

Pomax commented 8 years ago

awesome, thank you so much for all the hard work, I will try to merge this in and get a new version published on Thursday

genffy commented 8 years ago

np, my pleasure

Pomax commented 8 years ago

heya -- sorry it's been so long, I've only just gotten to being able to clear outstanding github issues, so I just tried pulling this and running npm test which looks to need a tiny fix (env = new habitat(); rather than env = habitat.load();) but other than that it seems to do the trick - do you still remember what the idea behind this PR was in terms of having both a "test" and a "testWP" which seems to not do any testing but just runs webpack?