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

v1.3.0 fails on require #42

Closed
seanewest opened this issue Aug 25, 2015 · 33 comments
Closed

v1.3.0 fails on require #42

seanewest opened this issue Aug 25, 2015 · 33 comments
Labels

Comments

@seanewest
Copy link

The latest release/tag and npm package, v1.3.0, has in package.json "main": "./build/server/cuid.js", but includes no folder build.

The package.json also still references the dist folder, which was included in v1.2.4, but is not in v1.3.0.

@jxson
Copy link

jxson commented Aug 25, 2015

This is a problem for me as well, the build folder is missing for cuid@1.3.0

Note: I caught this via CI I am sure lots of people are having this issue...

@nikuda
Copy link

nikuda commented Aug 25, 2015

Same problem. "main" is pointing to a non-existent file so you can't require cuid at all since 1.3.0.

@crabmusket
Copy link
Contributor

Another one here.

@seanewest seanewest changed the title v1.3.0 is missing dist/build folder v1.3.0 fails on require Aug 25, 2015
@mmckegg
Copy link

mmckegg commented Aug 25, 2015

📣

mmckegg added a commit to mmckegg/loop-drop-app that referenced this issue Aug 25, 2015
@ericelliott
Copy link
Collaborator

As @mmckegg has demonstrated, the temporary fix is to peg cuid @ 1.2.4 in your package.json dependencies list:

"cuid": "1.2.4"

I'm working on a more permanent fix right now. Sorry for the trouble! =)

@ericelliott
Copy link
Collaborator

If anybody can beat me to it, I'm happy to take a PR, too. I'll watch this thread while I'm working.

@nikuda
Copy link

nikuda commented Aug 25, 2015

Thanks @ericelliott. v1.2.5 works as well for what's it worth.

@ericelliott
Copy link
Collaborator

Yeah. Looks like I forgot to check that the build was configured properly. My bad. Sorry everybody. Fix landing soon. =)

@ericelliott
Copy link
Collaborator

Cool.. looks like the new build also breaks the API. Fixing that, too. Almost done folks! =)

@ericelliott
Copy link
Collaborator

Just figuring out the browser tests, now.

@ericelliott
Copy link
Collaborator

This is why you shouldn't try to code when you're sleep deprived. The long string of published updates was me trying to fix this problem when I should have been sleeping. I'm giving up for tonight.

In the meantime, I've rolled back the build outputs to the previous known working version.

In the interest of not breaking 100 projects again, I'll keep those known working versions there until I'm confident that the new build is working properly (complete with CI checks).

If anybody wants to take a crack at getting the new build to work, you're welcome to. I'm trying to use Zuul to automate cross browser tests on Sauce Labs so we can be more confident about future changes. That's not working for me... The browser sessions work, but it doesn't seem to see any test results.

😴

@ericelliott
Copy link
Collaborator

fyi, old (good) builds are in ./dist. New (broken) builds are in ./build. Have fun. =)

@ericelliott
Copy link
Collaborator

ping @therealklanni

@ericelliott
Copy link
Collaborator

Broken build error:

$ node
> var cuid = require('./node_modules/cuid/build/server-cuid.js');
TypeError: Cannot read property 'toString' of undefined
    at Object.<anonymous> (/Users/eric/Dropbox/dev/tmp/node_modules/cuid/build/server-cuid.js:161:27)
    at Object.module.exports (/Users/eric/Dropbox/dev/tmp/node_modules/cuid/build/server-cuid.js:177:31)
    at __webpack_require__ (/Users/eric/Dropbox/dev/tmp/node_modules/cuid/build/server-cuid.js:20:30)
    at Object.defineProperty.value (/Users/eric/Dropbox/dev/tmp/node_modules/cuid/build/server-cuid.js:59:20)
    at __webpack_require__ (/Users/eric/Dropbox/dev/tmp/node_modules/cuid/build/server-cuid.js:20:30)
    at Object.defineProperty.value (/Users/eric/Dropbox/dev/tmp/node_modules/cuid/build/server-cuid.js:40:18)
    at Object.<anonymous> (/Users/eric/Dropbox/dev/tmp/node_modules/cuid/build/server-cuid.js:43:10)
    at Module._compile (module.js:460:26)
    at Object.Module._extensions..js (module.js:478:10)
    at Module.load (module.js:355:32)

@jxson
Copy link

jxson commented Aug 25, 2015

@ericelliott thanks for rolling back, cuid@1.3.8 is working for my browserify builds now.

@ericelliott
Copy link
Collaborator

👍

@therealklanni
Copy link
Contributor

I'll need to check what changed between 1.2.x and 1.3.0. Will be all over this tonight.

@ericelliott
Copy link
Collaborator

@therealklanni What changed is that I published the modernization / modularization changes without double checking that all the testing was ready to go. There were no client-side tests at all, and I wasn't able to get the server-side import to work, either last night. I made some changes last night you may want to look at. Maybe you'll have better luck understanding what's going wrong with the builds.

But this much is clear after the mess yesterday: We need to be testing the build results, not just the source modules. =)

I'm going to insist that we keep the build in source control, too. I like the comfort of knowing there's always a known working build people can download from the repo.

@therealklanni
Copy link
Contributor

@ericelliott I'm looking into it now. It appears the browser tests just hang forever. Looking into it further it appears the sauceReporter.js was not being imported, so I've tried importing that and passing tape to it, however it still appears to be hanging. Will update you when I know more.

Agreed, we should test the build.

I didn't realize we weren't committing the builds, we definitely should be.

@ericelliott
Copy link
Collaborator

I'm looking into it now. It appears the browser tests just hang forever. Looking into it further it appears the sauceReporter.js was not being imported, so I've tried importing that and passing tape to it, however it still appears to be hanging. Will update you when I know more.

It shouldn't be required with Zuul... Zuul's whole thing is "we do this sauce connection/reporting stuff for you."

sauceReporter was my backup plan if we can't get Zuul to work. We'll need to report manually with the REST API if we can't get it working. :)

@therealklanni
Copy link
Contributor

@ericelliott OK, well maybe that wasn't the problem. In any case, I moved on to looking into the build issue.

I just figured out (part of) the issue with the server build at least. The webpack config wasn't specifying the library or libraryTarget. Adding those for the final case of the ternary there fixed at least part of the issue. Doing a little more digging now.

@ericelliott
Copy link
Collaborator

BTW, if we do have to use the sauceReporter, we have to add an extra test at the end called 'complete' to trigger the second condition. I couldn't figure out any other way to know when all the tests are finished. :| See source.

There must be a way if zuul ever worked for anybody, but I didn't find it last night.

@therealklanni
Copy link
Contributor

Looks like I resolved the issue with the server build at least. Need to publish a fix to node-fingerprint and then I'll PR the cuid server build fix.

@therealklanni
Copy link
Contributor

@ericelliott However, we have another issue with testing the build instead of the unbuilt code. When Webpack builds the source, the process.pid property doesn't exist when running the code. I have a workaround for that, but maybe there's a better (more correct) way to resolve this?

@therealklanni
Copy link
Contributor

@ericelliott we have a good server build now, pending your review. I'll move on to working on the client build next.

@ericelliott
Copy link
Collaborator

Anybody else want to download the latest and give it a test? This isn't published on npm yet, so you'll need to wire it up manually.

@ericelliott
Copy link
Collaborator

@therealklanni What's the next step?

@therealklanni
Copy link
Contributor

@ericelliott I think I was working on the client build, but I've been getting pulled in other directions. I'll check to see where I was at with that.

@ericelliott
Copy link
Collaborator

=)

@therealklanni
Copy link
Contributor

Sorry, haven't gotten around to it yet. It's the last 2 weeks of the bootcamp, so my students need my full attention, and trying to plan Thanksgiving 😩 ... I haven't forgotten about it, though.

@therealklanni
Copy link
Contributor

Looks like all that needs to be done is to write browser tests for browser-fingerprint

@ericelliott
Copy link
Collaborator

ping @therealklanni

@ericelliott
Copy link
Collaborator

Should be fixed. Reopen if you still have trouble.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants