fix usage as a commonjs module by evanshortiss · Pull Request #41 · feedhenry/fh-sync-js

evanshortiss · 2017-12-13T01:55:01Z

Work that should address #40. Need to test in my project, but the example is still working after this change.

evanshortiss · 2017-12-13T01:55:46Z

-};
+};
+
+require('./lawnchairHtml5FileSystem')(Lawnchair)


Instead of using concat, these are loaded via require now so browserify will pick them up.

evanshortiss · 2017-12-13T01:56:43Z

      crypto: {
        src:[
          "libs/cryptojs/cryptojs-core.js",
-          "libs/cryptojs/cryptojs-enc-base64.js",


Most of these were referenced but no longer exist.

evanshortiss · 2017-12-13T01:57:05Z

      }
    },
    concat: {
-      lawnchair: {


This is no longer required thanks to browserify.

Yeah. This is just leftover from js-sdk. Sadly we need to still bundle lawnchair as source code weren't released for the last couple years and we made some fixes directly upstream to maintain it :)

evanshortiss · 2017-12-13T01:58:28Z

@@ -1,105 +1,107 @@
-Lawnchair.adapter('memory', (function(){
+module.exports = function (Lawnchair) {


Formatting got a little wild here. Should we run add standard and use standard --fix or similar to keep things consistent? Maybe an .editorconfig too?

yeah, or we could move them out into its own module. It shouldn't be in this repo I think.

I think these should reside in require('lawnchair').memoryAdapter.

I'm not sure where the code for these adapters come from originally, since I couldn't find them standalone on npm or on the original repository for lawnchair...

For styling I think we might be a litle closer to semi-standard than standard, just because semicolons are a hot topic. We have a recommended set of styles for our own code somewhere but I couldn't find it right now, going to look further.

EDIT: so it seems the code for most of these adapters is already inside the original repo, just not exposed by the module as commonjs.

I'm not sure where the code for these adapters come from originally, since I couldn't find them standalone on npm or on the original repository for lawnchair...

They were renamed, removed etc. long time ago.
We using some really old ones. Here is tag that has some of them:
https://github.com/brianleroux/lawnchair/tree/0.6.4/src/adapters

evanshortiss · 2017-12-13T02:05:15Z

Fix confirmed in ng4 Ionic project that uses this module.

wei-lee

I think for now this is ok. But in the long term, I suggest we move Lawnchair to its own repo, or get rid of it completely.

wtrocki · 2017-12-13T12:19:25Z

@evanshortiss Thank you so much for starting and providing solution for this important issue.
I have reviewed your contribution and added other interesting parties to have full overview about directions we should take to resolve this issue completely. Your fix resolves original issue, but for sync maintainers it's also important to make sure that we all are aware of the possible impact and changes this will trigger.

We talked about this issue a lot with @paolobueno

Background

I'm going to provide some background to explain why we have some external source code in our module. We put lawnchair in our code because original library weren't published for years:
https://github.com/brianleroux/lawnchair/releases

Every time browser api changes we making fix in upstream and backport changes in lib folder to make it compatible. For example:
brianleroux/lawnchair#229

Lawnchair doesn't support common.js modules - extensions are just mounted to global var. Concat were used as hack and were causing issues from the beginning.

Suggested fix

In my opinion the best course of the action will be to install Lawnchair as dependency.

We have two options:

Wait for maintainer to publish
Paolo done fix to include src folder in the module.
We asked for module to be released and waiting for response:
Include src/ in published package brianleroux/lawnchair#233

If you guys can +1 this idea this may definitely help.
When getting package this way we will still need to figure out how to enable browserify.

Bring Lawnchair under our umbrella and release it to npm together with all storage drivers. Then we can just have it as dependency in sync and prevent from any issues. This will also allow us to move to more modern web common.js engines like webpack (this was spiked by @paolobueno )

Current fix

This fix may be the first step to extract Lawnchair out of the sync module. However I do not understand why we committing both source and generated code. We can make assumption that this is library and remove source code as we no longer have ability to concatenate that.
I also noticed some changes in actual library code - did we done update?
Making changes in this code will also complicate future updates with upstream - it's not that big issue IMHO but it's definitely something to be aware of.

wtrocki

Agreeing with Wei, but I would like to understand why we have both generated content and source code before we merge this PR.

wei-lee · 2017-12-13T12:27:48Z

yeah, I think the generated file can be removed now. It's not required anymore.

paolobueno · 2017-12-13T13:17:38Z

Actually the author has shown himself willing to merge changes and do a release. I'd PR'd from a personal fork but we could definitely have one in the feedhenry org to move forward faster if we wish to.

The minimal fix in brianleroux/lawnchair#233 is still not enough, it's missing practically what @evanshortiss is doing here, making the adapters commonjs modules instead of them expecting to be run on top of Lawnchair as a global.

evanshortiss · 2017-12-13T17:16:31Z

Agreed @wtrocki. If it is published to npm that'd be great and solve our headache here so long as what @paolobueno says is also addressed 👍 Have added +1

Bring Lawnchair under our umbrella and release it to npm together with all storage drivers. Then we can just have it as dependency in sync and prevent from any issues

I tried to help get it published a long time ago actually! So if it doesn't see activity soon then maybe you're right, we could publish it under @rhm/lawnchair or a similar npm org.

wtrocki · 2017-12-18T12:31:46Z

As this may cause some problems in automation etc. I have done prerelease to test it in together with fh-js-sdk and other projects:

fh-sync-js@1.2.2-pre41

@evanshortiss We kinda need consensus here. I think that we may integrate this fix and then look towards using our own version of lawnchair. Do you think we should close this PR and go directly to our version?

@david-martin - What's your take on this?

wtrocki · 2017-12-18T12:39:06Z

@evanshortiss - I can confirm that this fixes problem with browserify.
Works fine and passes all the tests.

evanshortiss · 2017-12-18T18:34:25Z

@wtrocki I'm happy with either approach really.

I suppose the upside of merging this is that it fixes the module temporarily until we can look into using our own version.

wtrocki · 2017-12-18T19:10:47Z

@@ -0,0 +1,1354 @@
+/**


Can we remove this file?

wtrocki · 2017-12-18T19:11:49Z

@evanshortiss - Let's merge it then. Just one small thing to clear that out.

fix usage as a commonjs module

3a01787

evanshortiss commented Dec 13, 2017

View reviewed changes

evanshortiss requested a review from wtrocki December 13, 2017 02:00

evanshortiss commented Dec 13, 2017

View reviewed changes

wtrocki requested a review from paolobueno December 13, 2017 10:47

wtrocki self-assigned this Dec 13, 2017

wtrocki requested review from david-martin and wei-lee December 13, 2017 11:56

wei-lee approved these changes Dec 13, 2017

View reviewed changes

wtrocki suggested changes Dec 13, 2017

View reviewed changes

wtrocki mentioned this pull request Dec 18, 2017

Bump sync version feedhenry-raincatcher/raincatcher-core#201

Merged

wtrocki reviewed Dec 18, 2017

View reviewed changes

Comment thread libs/generated/lawnchair.js

@@ -0,0 +1,1354 @@

/**

wtrocki Dec 18, 2017 •

edited

Loading

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

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

Can we remove this file?

wtrocki approved these changes Jan 31, 2018

View reviewed changes

wtrocki requested a review from aidenkeating January 31, 2018 12:20

wtrocki merged commit 7ffbb90 into master Jan 31, 2018

wtrocki deleted the commonjs-fix branch January 31, 2018 12:23

		@@ -1,105 +1,107 @@
		Lawnchair.adapter('memory', (function(){
		module.exports = function (Lawnchair) {

Uh oh!

Conversation

evanshortiss commented Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evanshortiss Dec 13, 2017

Choose a reason for hiding this comment

Uh oh!

evanshortiss Dec 13, 2017

Choose a reason for hiding this comment

Uh oh!

evanshortiss Dec 13, 2017

Choose a reason for hiding this comment

Uh oh!

wtrocki Dec 13, 2017

Choose a reason for hiding this comment

Uh oh!

evanshortiss Dec 13, 2017

Choose a reason for hiding this comment

Uh oh!

wei-lee Dec 13, 2017

Choose a reason for hiding this comment

Uh oh!

paolobueno Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wtrocki Dec 13, 2017

Choose a reason for hiding this comment

Uh oh!

evanshortiss commented Dec 13, 2017

Uh oh!

wei-lee left a comment

Choose a reason for hiding this comment

Uh oh!

wtrocki commented Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Background

Suggested fix

Current fix

Uh oh!

wtrocki left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wei-lee commented Dec 13, 2017

Uh oh!

paolobueno commented Dec 13, 2017

Uh oh!

evanshortiss commented Dec 13, 2017

Uh oh!

wtrocki commented Dec 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wtrocki commented Dec 18, 2017

Uh oh!

evanshortiss commented Dec 18, 2017

Uh oh!

wtrocki Dec 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wtrocki commented Dec 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

evanshortiss commented Dec 13, 2017 •

edited

Loading

paolobueno Dec 13, 2017 •

edited

Loading

wtrocki commented Dec 13, 2017 •

edited

Loading

wtrocki left a comment •

edited

Loading

wtrocki commented Dec 18, 2017 •

edited

Loading

wtrocki Dec 18, 2017 •

edited

Loading