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.

Sign up for GitHub

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

Jump to bottom

Better build process. Fix #1238, #1232 #1248

Merged
merged 2 commits into from Jun 20, 2017
Merged

Better build process. Fix #1238, #1232 #1248

merged 2 commits into from Jun 20, 2017

Conversation

tnajdek
Copy link
Member

@tnajdek tnajdek commented Jun 19, 2017

This is a new, re-written build process to make it less cumbersome. Here are the major changes:

@dstillman
Copy link
Member

This is great. Just for house style, can we rename trailing abbreviations to be capitalized (getJs -> getJS) and add spaces after statements (if (, while (, for () to differentiate from functions?

dstillman
dstillman reviewed Jun 19, 2017
View reviewed changes
scripts/utils.js Outdated
return path.relative(path.join(ROOT, dirName), path.join(ROOT, f));
}

const formatDirsforMatcher = dirs => {
Copy link
Member

Choose a reason for hiding this comment

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

Capital F

@dstillman
Copy link
Member

dstillman commented Jun 19, 2017 •
edited

And should probably log a warning for earlier Node versions, so people know they're getting a suboptimal experience.

dstillman
dstillman reviewed Jun 19, 2017
View reviewed changes
scripts/utils.js Outdated
if('isError' in global && global.isError) {
return;
}
console.log(`${colors.blue(`[${operation}]`)} ${sourcefile} -> ${outfile}`);
Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove the -> ${outfile}. All it's ever adding is build/ at the beginning, right? Easier to read without that.

@dstillman
Copy link
Member

The npm i on Travis is failing with Cannot find module 'array-union'. I actually saw that locally, but then I think I cleared node_modules to fix it. But clearing the cache on Travis doesn't seem to be doing the trick.

@tnajdek
Better build process. Fix zotero#1238, zotero#1232
63047c4 
* Remove gulp, replace with custom scripts
* Symlink entire dirs where possible
* Significantly speed up subsequent builds
* Watch process now observes new/removed files, not only changed
* Add ignoreMask, exclude all files with names starting with a #
* Better logging during builds
@tnajdek
Update travis.yml to use new, non-gulp-based build
9250144
@tnajdek
Copy link
Member Author

tnajdek commented Jun 20, 2017 •
edited

Updated PR:

  • Adjusted naming convention & keyword-spacing
  • Tweaked signatures code to work correctly in node 7.x
  • Simplified onProgress output, unless NODE_ENV=debug
  • Introduced ignoreMask for easy file-exclusion

The npm i on Travis is failing with Cannot find module 'array-union'. I actually saw that locally, but then I think I cleared node_modules to fix it. But clearing the cache on Travis doesn't seem to be doing the trick.

I'm struggling to reproduce that locally. I can't think of the reason other than travis cache.

@dstillman
Copy link
Member

OK, working after clearing the master cache. (Previously I only cleared the PR cache. Not sure why that didn't work.)

@dstillman dstillman merged commit b53fabb into zotero:master Jun 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dstillman dstillman dstillman left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@tnajdek @dstillman