launchdarkly / js-client-sdk

LaunchDarkly Client-side SDK for Browser JavaScript
Other
112 stars 65 forks source link

Prepare the `next` branch for release #57

Closed apucacao closed 5 years ago

apucacao commented 6 years ago

53 now lives on the branch next. Once the few items below are fixed, we'll publish to npm with the next tag. (So npm install ldclient-node@next works.)

Tasks:

apucacao commented 6 years ago

@tdeekens I'm collecting things that need to happen before we can merge next into master here. I've also published the @next version to npm.

tdeekens commented 6 years ago

Thanks! I am trying to follow: we should address the things stated above before we publish to @next or did you already?

apucacao commented 6 years ago

I just published @next to npm. I think we should fix those issues before we can merge next into master.

tdeekens commented 6 years ago

Ah, yeah sure. Makes total sense. I am only "worried" about getting the tests to run properly.

tdeekens commented 6 years ago

Little update

Released the launchdarkly-adapter at flopflip to @next with a js-client from @next

Building the js-client from ESM to CJS works fine. Bundle sizes have not changed at all interestingly. Just wanted to make sure we don't leak anything more to clients.

From @latest:
Destination: dist/@flopflip-launchdarkly-adapter.cjs.js
Bundle size: 6.07 KB, Gzipped size: 1.92 KB

to @next
Destination: dist/@flopflip-launchdarkly-adapter.cjs.js
Bundle size: 6.07 KB, Gzipped size: 1.92 KB

Tested the js-client from the @next release

This currently causes problems due to the exports changing from the CJS to ESM style in the diff

- module.exports = {
-   initialize: initialize  
- };

+ export default {
+   initialize,
+ };

as this breaks and does not work any more

import { initialize } from 'ldclient-js';

return initialize(clientSideId, user);

and would have to be rewritten to

import ldClient from 'ldclient-js';

return ldClient.initialize(clientSideId, user);

I guess we could also change it to a named export to potentially avoid breaking changes towards users of the library.

apucacao commented 6 years ago

I think ensuring the library is backward-compatible would be ideal. Using a named export like you suggest would ensure that, right?

tdeekens commented 6 years ago

Yes, I think it should. I can test it on my integration branch at flopflip to make sure. Some set of linters might not get too excited about it as they would suggest to prefer a default export in that case.

Update, https://github.com/launchdarkly/js-client/pull/63 addresses that.

tdeekens commented 6 years ago

That leaves us "only" with the tests locally no running. My karma knowledge gotten a bit rusty over the last year. Do you have any idea what might cause this?

26 12 2017 17:41:28.833:DEBUG [web-server]: Instantiating middleware
26 12 2017 17:41:28.834:DEBUG [reporter]: Trying to load reporter: mocha
26 12 2017 17:41:28.836:DEBUG [reporter]: Trying to load color-version of reporter: mocha (mocha_color)
26 12 2017 17:41:28.836:DEBUG [reporter]: Couldn't load color-version

I don't have much of a clue where to start digging. Personally I'd like the project to be moved over to Jest and Circle@2 in the future. What are your plans there?

apucacao commented 6 years ago

Hmm, not sure what's going on…

We switched to Jest for our other projects, and it would be great if we could do this here as well. Does switching to Jest require that we also switch to circle 2?

tdeekens commented 6 years ago

Nope it doesn‘t. Switching to Jest would mean something like:

apucacao commented 6 years ago

I'll take a look to see if there any utils that could help with mocking an HTTP server for Jest.


Update: On second thought: could we keep using sinon's fake http server for now? We can still use jest.fn for simple mocks, but that way we can defer the sinon question?

apucacao commented 6 years ago

/cc @brooswit

tdeekens commented 6 years ago

That would work. Maybe it's worth not having sinon as a global but just import it. That's another thing however.

apucacao commented 6 years ago

That was successful: we're not using jest with sinon in a few places. All tests pass, all linting errors are gone.

I'm going to publish the latest @next to npm so that we can do some real-life testing.

Update npm install ldclient-js@next will now install the latest version of this branch.

tdeekens commented 6 years ago

Cool. I can do some integration testing with flopflip tomorrow to see if it all works as expected.

tdeekens commented 6 years ago

There seems to be an problem with the bundling still. In another project, using flopflip and webpack (which should be unrelated) I get Uncaught Error: Cannot find module "./EventProcessor". Whereas the overall error seems to be affecting more modules

var EventProcessor = __webpack_require__(!(function webpackMissingModule() { var e = new Error("Cannot find module \"./EventProcessor\""); e.code = 'MODULE_NOT_FOUND'; throw e; }()));
var EventEmitter = __webpack_require__(!(function webpackMissingModule() { var e = new Error("Cannot find module \"./EventEmitter\""); e.code = 'MODULE_NOT_FOUND'; throw e; }()));
var EventSerializer = __webpack_require__(!(function webpackMissingModule() { var e = new Error("Cannot find module \"./EventSerializer\""); e.code = 'MODULE_NOT_FOUND'; throw e; }()));
var GoalTracker = __webpack_require__(!(function webpackMissingModule() { var e = new Error("Cannot find module \"./GoalTracker\""); e.code = 'MODULE_NOT_FOUND'; throw e; }()));
var Stream = __webpack_require__(!(function webpackMissingModule() { var e = new Error("Cannot find module \"./Stream\""); e.code = 'MODULE_NOT_FOUND'; throw e; }()));
var Requestor = __webpack_require__(!(function webpackMissingModule() { var e = new Error("Cannot find module \"./Requestor\""); e.code = 'MODULE_NOT_FOUND'; throw e; }()));
var Identity = __webpack_require__(!(function webpackMissingModule() { var e = new Error("Cannot find module \"./Identity\""); e.code = 'MODULE_NOT_FOUND'; throw e; }()));
var utils = __webpack_require__(!(function webpackMissingModule() { var e = new Error("Cannot find module \"./utils\""); e.code = 'MODULE_NOT_FOUND'; throw e; }()));
var messages = __webpack_require__(!(function webpackMissingModule() { var e = new Error("Cannot find module \"./messages\""); e.code = 'MODULE_NOT_FOUND'; throw e; }()));
var store = __webpack_require__(!(function webpackMissingModule() { var e = new Error("Cannot find module \"./store\""); e.code = 'MODULE_NOT_FOUND'; throw e; }()));
var errors = __webpack_require__(!(function webpackMissingModule() { var e = new Error("Cannot find module \"./errors\""); e.code = 'MODULE_NOT_FOUND'; throw e; }()));

Are you experiencing the same?

apucacao commented 6 years ago

Yeah I get similar errors. Not sure what's going on yet.

apucacao commented 6 years ago

I get these errors:

* ./EventEmitter in ./node_modules/ldclient-js/dist/ldclient.umd.js
* ./EventProcessor in ./node_modules/ldclient-js/dist/ldclient.umd.js
* ./EventSerializer in ./node_modules/ldclient-js/dist/ldclient.umd.js
* ./GoalTracker in ./node_modules/ldclient-js/dist/ldclient.umd.js
* ./Identity in ./node_modules/ldclient-js/dist/ldclient.umd.js
* ./Requestor in ./node_modules/ldclient-js/dist/ldclient.umd.js
* ./Stream in ./node_modules/ldclient-js/dist/ldclient.umd.js
* ./errors in ./node_modules/ldclient-js/dist/ldclient.umd.js
* ./messages in ./node_modules/ldclient-js/dist/ldclient.umd.js
* ./store in ./node_modules/ldclient-js/dist/ldclient.umd.js
* ./utils in ./node_modules/ldclient-js/dist/ldclient.umd.js

I'm not sure why it's trying to load those modules since it should all be bundled in one file?

apucacao commented 6 years ago

Alright, I believe I fixed those errors. I'm going to start testing in production again, but as far as I could tell, the different builds are working now.

apucacao commented 6 years ago

Published beta.3 with a couple more fixes. We're using it in one production app, with a second one shortly.

@tdeekens If you get a chance, I'd love to see how the latest beta — 1.5.1-beta.3 — does with your test suite.

tdeekens commented 6 years ago

Will check Monday or early next week. Thanks!

tdeekens commented 6 years ago

Alright. I checked the js-client with flopflip in our production app. All works fine an as expected. I linked in the third beta. Everything compiles nicely and toggling works just fine as before.

Was there anything else we should have kept an eye on?

apucacao commented 6 years ago

Do you know which format you tested? umd?

Now I just want to make sure we've covered all 3 output formats — umd, es and cjs — and we should be good to go.

tdeekens commented 6 years ago

Running in the app it should have been ESM.

apucacao commented 6 years ago

Alright, I've verified that the build produces working UMD, CommonJS and ES bundles. I believe this is ready to go now.

tdeekens commented 6 years ago

Awesome. I guess just the general "pre-big-change"-uncertainty remains :)

tdeekens commented 6 years ago

Are we gonna do this? I don‘t think waiting will stop master from running away. I assume everything from master would need careful porting.

apucacao commented 6 years ago

Hey @tdeekens, I actually merged the next branch earlier this week. We should be able to release a new version on Monday!