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

Remove devsetup and clean up bundling #103

Closed
wants to merge 5 commits into from

Conversation

rdeits
Copy link
Collaborator

@rdeits rdeits commented May 4, 2018

Replaces #102, fixes #101

I was able to get rid of the devsetup() step entirely, and get rid of its associated node_modules folder in deps. Since we were already doing a npm install in the bundlejs() step, I don't think there's much reason for a separate devsetup().

I've tightened the version restrictions in package.json and checked in package-lock.json so we'll have a clear idea when package versions change. Unfortunately, this doesn't guarantee future compatibility, because npm install overwrites, rather than respecting, package-lock.json. It looks like making that guarantee will be possible with npm 5.7.1 using the npm ci command: npm/npm#18286 (comment)

By the way, bundlejs(watch=true) still works after this change 🙂

Also, we should check if this helps with #79 and #94

@codecov-io
Copy link

codecov-io commented May 4, 2018

Codecov Report

Merging #103 into master will increase coverage by 0.15%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #103      +/-   ##
==========================================
+ Coverage    60.8%   60.95%   +0.15%     
==========================================
  Files          14       14              
  Lines         398      397       -1     
==========================================
  Hits          242      242              
+ Misses        156      155       -1
Impacted Files Coverage Δ
src/devsetup.jl 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 204f580...7fe5400. Read the comment docs.

@shashi
Copy link
Member

shashi commented May 5, 2018

I'm not sure now if this still supersedes #99. Let me know and I'll merge the right PR.

Thanks!

@rdeits
Copy link
Collaborator Author

rdeits commented May 5, 2018

Yeah, sorry, I tried to keep the changes here separate from #99 but it all got kind of mixed together in the end. I'll close this so we can focus on a single set of changes.

@rdeits rdeits closed this May 5, 2018
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.

Regenerating javascript bundle breaks scopes with imports
3 participants