issue 597 :grunt watch files for dist/bramble.js #906
Conversation
peiying16
changed the title
|
Now, it takes me 10 seconds to build this file. |
|
@peiying16 great to see you back on Thimble/Brackets again! I am going to get @gideonthomas to do a review of this. |
|
@humphd |
gideonthomas
left a comment
gideonthomas
left a comment
There was a problem hiding this comment.
Excellent optimization by adding file watching @peiying16! I'm requesting two minor changes, but other than that I think the approach here is solid.
Gruntfile.js
Outdated
| 'src/bramble/client/**/*.js', | ||
| 'src/bramble/thirdparty/**/*.js', | ||
| 'src/bramble/ChannelUtils.js', | ||
| 'thirdparty/filer/dist/filer.min.js' |
There was a problem hiding this comment.
let's remove this and simplify the files to be just ['src/bramble/**/*']
There was a problem hiding this comment.
@gideonthomas The bramble.js is built from https://github.com/mozilla/brackets/blob/master/Gruntfile.js#L228-L234
That's why I want to watch all these files. Do you want to change to ['src/bramble/**/*']
There was a problem hiding this comment.
yes please. We do not want to watch files in thirdparty because they will (should) never change.
Gruntfile.js
Outdated
| 'src/bramble/ChannelUtils.js', | ||
| 'thirdparty/filer/dist/filer.min.js' | ||
| ], | ||
| tasks: ['requirejs:iframe'] |
There was a problem hiding this comment.
let's instead change the task to be grunt build-browser as it should only build the iframe api if there were changes to those files and grunt build-browser should handle that.
There was a problem hiding this comment.
@gideonthomas
The build-browser takes too much time to process. If we only want to compiler the bramble.js file. We only need to run requirejs:iframe. See this link.
https://github.com/mozilla/brackets/blob/master/Gruntfile.js#L226-L255
The purpose of this PR is that we can reduce build time by running requirejs:iframe instead of build-browser.
What do you think?
There was a problem hiding this comment.
sorry, I meant just change it to grunt build-browser-dev, that should only run a single task and should be fairly quick.
peiying16
changed the title
|
@gideonthomas Please review. |
gideonthomas
left a comment
gideonthomas
left a comment
There was a problem hiding this comment.
looks great! Thanks @peiying16!
package.json
Outdated
| "unlocalize": "rimraf src/nls && git checkout -- src/nls", | ||
| "postinstall": "grunt install", | ||
| "build": "grunt build-browser", | ||
| "build": "grunt build-browser && grunt watch:bramble", |
There was a problem hiding this comment.
actually, I think we might just have incorrect documentation for this.
Why don't we just set build to be grunt build-browser-dev,
add another script called "watch:api" that runs grunt watch:bramble
install a new package called npm-run-all
and change start to be run-p server watch:api || true
That way, you basically never have to run npm run build and only always run npm start which will do the file watching for you.
|
@gideonthomas Because we don't need to run |
gideonthomas
left a comment
gideonthomas
left a comment
There was a problem hiding this comment.
This is very close @peiying16. Thanks for sticking with it. I've requested a few changes for compatibility. One other thing you need to do is to update the documentation:
- Remove step 3 in https://github.com/mozilla/brackets#how-to-setup-bramble-brackets-in-your-local-machine. We don't need it anymore.
- Modify the comment in step 4 to also say that if any changes are made in the
srcdirectory, just refresh the page hosting Bramble in your browser to reflect those changes.
package.json
Outdated
| "localize-dist": "node scripts/properties2js dist", | ||
| "unlocalize": "rimraf src/nls && git checkout -- src/nls", | ||
| "postinstall": "grunt install", | ||
| "build": "grunt build-browser", |
There was a problem hiding this comment.
grunt build-browser-dev is just an augmented form of grunt-build-browser and realistically, grunt build-browser shouldn't be run at any point, so I would say change it to that.
package.json
Outdated
| "server": "http-server -p 8000 --cors", | ||
| "prestart": "npm run localize && grunt build-browser-dev", | ||
| "start": "npm run server || true", | ||
| "start": "run-p server watch:api || true", |
There was a problem hiding this comment.
darn, I just noticed that the || true doesn't really work well with run-p on mac. To fix that, let's break this into two different scripts:
"start:server": "run-p server watch:api",
"start": "npm run start:server || true"
|
@gideonthomas Done. Thanks for your review. |
|
This is awesome @peiying16, thanks for getting this done! |
3 participants
#597
npm run buildtook me 15 minutes to process on my i3 device.It runs the following tasks
https://github.com/mozilla/brackets/blob/master/Gruntfile.js#L577-L583
However, only the iframe task needs to run when we change
src/bramble/client/main.jshttps://github.com/mozilla/brackets/blob/master/Gruntfile.js#L226-L255
Option: #1
So I watch all related files and add to
npm run buildThe problem is that we don't know when
npm run buildis done, and we need to open another window to runnpm start.Option: #2
I can add another script to package.json. When we need to change the related files, we can run this script to watch the files.
The problem is people don't know this script.
What option do you like? This pull request is done in option 1.