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

Upgrade to nwb 0.11.0 & fixes #39 (concat vendor/app scripts) #46

Closed
wants to merge 6 commits into from

Conversation

addyosmani
Copy link
Collaborator

This patchset upgrades us to the latest version of nwb and also moves us over to async loading in the single bundle generated by vendorBundle: false.

@insin
Copy link
Owner

insin commented Jun 10, 2016

LGTM

I want to check if there's a difference between what gets bundled using the react command vs. using the nwb command, as they have different presets. The react command was intended for standalone serving of an entry point sitting anywhere, but looks like it works just fine for projects too 👍

@addyosmani
Copy link
Collaborator Author

SGTM :) Thanks for taking a look! The one difference I did notice was what appeared to be gzipped files being output to the project root directory. I don't believe I changed anything else here that would cause that but I was unsure if that was an intended default now or something we could turn off (or a bug) :)

@NekR
Copy link
Collaborator

NekR commented Jun 10, 2016

Seems fine, but I wasn't able to figure out how nwb works, so no idea how it affects build. If @insin says it's okay, then it has to be okay :-)

@insin
Copy link
Owner

insin commented Jun 10, 2016

Er, yeah, that sounds weird, I'll check it out locally tonight.

@addyosmani
Copy link
Collaborator Author

@insin By any chance, were you able to repro what I was seeing? :) I'd be happy to switch back to the nwb command to see if those artefacts still get output with latest.

@addyosmani
Copy link
Collaborator Author

Once we've had a chance to investigate further, I'll update so #39 is tackled. Not super high priority atm.

@addyosmani
Copy link
Collaborator Author

@insin friendly ping just in case you get a chance to look at this :) If you're busy I can take a look at moving back to nwb over react

@insin
Copy link
Owner

insin commented Jul 13, 2016

@addyosmani thanks, on my radar again

Is there any way to get GitHub's Assigned Issues view to not include is:issue by default? I've started to appreciate the focus you can get by assigning stuff to yourself and only ever looking at that when you have some time to spare.

@insin
Copy link
Owner

insin commented Jul 14, 2016

I'm not seeing the gzipped file thing, but there are a couple of other issues I hit which I can add a commit to this branch for.

We do not want to switch to nwb for the build because one of the things it does by default is attempt to copy a public/ directory's contents to the build directory, which is nested under public/ with the current config 😸

Fixes:

- Location of sw-toolbox.js appears to have changed, so fixed it
- Add babel and babel-core dependencies (=nwb versions) to fix server.js with npm2

Tweaks:

- Use node_modules/.bin/ scripts instead of absolute path to tool CLI scripts
- Lint server.js
- Pinned and sorted dependencies
@addyosmani
Copy link
Collaborator Author

@insin I think you might be able to use a bookmark to get the desired filtering there. For example this gives you the view you were after I believe. If I misunderstood, holler 📯

Thanks for the extra commits and tweaks to this branch! Looks like it requires a little rebasing against master. Will take a look at that.

"serve": "./node_modules/.bin/nwb serve",
"precache": "./node_modules/sw-precache/cli.js --root=public --config=sw-precache-config.json"
"serve": "nwb serve",
"precache": "sw-precache --root=public --config=sw-precache-config.json"
},
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I may have needed to use these paths to get the Node on GAE deploys happy again. I'll try it out with these changes and see if we run into any trouble at all.

@insin
Copy link
Owner

insin commented Jul 16, 2016

I genuinely forgot that bookmarks are A Thing 😜

@addyosmani
Copy link
Collaborator Author

Haha. Okay, soooo..updated this to a merge-friendly state. Local builds are all good.

I am still seeing those mysterious .json.gzip files in my builds, even with latest nwb installed. If however you aren't able to repro, I'll assume I may need to tinker with my nwb versions a little to see what's causing this.

screen shot 2016-07-16 at 09 21 07

Otherwise looks good to go.

@insin
Copy link
Owner

insin commented Jul 16, 2016

It looks like those are caused by the cacheDirectory setting in babel-loader, which nwb turns on with true config:

dvdzkwsk/react-redux-starter-kit#579

@addyosmani
Copy link
Collaborator Author

Cheers for the info! Is there a way we could explicitly set cacheDirectory via nwb and see if that changes things on Mac? I do think there's probably a weird OS-specific bug here that be more of an upstream problem.

@insin
Copy link
Owner

insin commented Jul 16, 2016

Are you running as root? Searching around it seems like that results in the cwd being the temp dir.

Edit: npm/npm#4531

@addyosmani
Copy link
Collaborator Author

Ah. That may well be it. I'm working on a Goog corp machine with some extra root weirdness applied to npm to work with our setup. If it's a known issue upstream there I can resolve this locally and we should be good to merge these changes.

@addyosmani
Copy link
Collaborator Author

This PR has been replaced with the work that just landed in master updating us to more recent versions of nwb and deps as well as adding in route-based code-splitting. Closing this partic PR off as it's outdated now.

@addyosmani addyosmani closed this Oct 13, 2016
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.

3 participants