Make cssjanus.js work in a browser and compatible with closure compiler.

[scriby]

This will help with an integration I'm trying to do at Google.

Thanks for taking a look!

[scriby]

I didn't indent here to try to keep the blame clean. Let me know if you'd like me to indent the contents here anyway.

[scriby]

Ended up taking out the IIFE due to linter restrictions.

[Krinkle]

@scriby Thanks for raising this issue. The current implementation is indeed tailored specifically towards use in Node.js, and does not consider the browser environment.

As you have found, the CSSJanus library itself is dependency-free and should work without issue in any JavaScript-based environment. The only issue is the export mechanism.

I ran into this before with the demo, but found another way for that (see https://cssjanus.github.io/ and index.html).

Regarding the actual change proposal, I would prefer to use the export pattern rather than conditional wrapping of the entire code. Could you update your pull request to use that pattern instead? For an example, see wikimedia/oojs:/src/export.js.

[Krinkle]

I haven't seen this particular way of exporting a module before. If this was based on something else, I'd appreciate a link so I can read more about it.

For Wikimedia projects, we usually follow the following pattern:

  if ( typeof module !== 'undefined' && module.exports ) {
    module.exports = Thing;
  } else {
    this.Thing = Thing;
  }

Or, if there are additional members that should remain private:

( function ( global ) {

  // ...

  if ( typeof module !== 'undefined' && module.exports ) {
    module.exports = Thing;
  } else {
    global.Thing = Thing;
  }
}( this  ) );

The closure would avoid exposing the other variables and classes to the browser scope, which seems undesirable.

I'm hoping Closure Compiler will know to not mangle the transform key of the object literal retuned from CSSJanus when it sees that it is exported publicly. I would prefer not to have quoted keys for Closure Compiler only.

If the quoting is needed, I wonder if it would work to quote the key directly on line 308. That would avoid the need for re-assignment here.

[scriby]

Unfortunately closure minifies everything, even things that are exported (at least with the build settings Google uses)! It is possible to quote transform at the place it's defined, but that triggered a linter error so I went with the other pattern (which also triggers a linter error, haha).

These changes are a little weird, so I think it would also be ok if we just maintained them as patches within the Google codebase that get applied before building.

[Krinkle]

I think using a quoted key for "transform" (line-disabling the linter) at the place its declared, would be a fine patch.

For the global variable we may still need a different patch, though. But I'd be fine with using a quoted index for that as well. Actually, I think we should just in general make the global variable export explicit. Right now there is no code for that because it's intended to run in Node.js.

Perhaps something like this?

if ( typeof module !== 'undefined' && module.exports ) {
  // ..
} else {
  this[ "cssjanus" ] = cssjanus;
}

And as part of that, also making the entire file privately scoped so that both with and without Closure Compiler, the other internal classes don't become global variables.

Adding an IFFY around a Node.js module seems odd though, so we may want to do that as part of a pre-publish build step for use in local development and in the npm-published package (something like dist/cssjanus.js for browsers).

[scriby]

I don't think we can use "this" to assign the cssjanus global variable because it won't be correct when wrapped in the IIFE. It's also not supported in strict mode, which we might want to be aiming for.

[Krinkle]

I think using a quoted key for "transform" (line-disabling the linter) at the place its declared, would be a fine patch.

For the global variable we may still need a different patch, though. But I'd be fine with using a quoted index for that as well. Actually, I think we should just in general make the global variable export explicit. Right now there is no code for that because it's intended to run in Node.js.

Perhaps something like this?

if ( typeof module !== 'undefined' && module.exports ) {
  // ..
} else {
  this[ "cssjanus" ] = cssjanus;
}

And as part of that, also making the entire file privately scoped so that both with and without Closure Compiler, the other internal classes don't become global variables.

Adding an IFFY around a Node.js module seems odd though, so we may want to do that as part of a pre-publish build step for use in local development and in the npm-published package (something like dist/cssjanus.js for browsers).

[scriby]

I attempted to account for the feedback, but not quite sure if I got it quite like you wanted. (I couldn't use "this" to export the variable due to a couple things I commented on in the other review).

I went ahead and put the IIFE in, but just let me know if you'd like it back out. I don't really think it's too big of a problem for the node use case, but if you wanted to keep the blame cleaner I could see an argument against it. I am also fine just patching the IIFE in on the Google side.

[Krinkle]

@scriby Yeah, I'd prefer not to have the IIFE in the Node source, it seems counter-intuitive given Node has files locally scoped by default. I was thinking of adding it in a build step.

If you could remove the IIFE, I'll go ahead and land this. We can look at the build step in a later change.

[scriby]

IIFE is out, thanks!

Resolved

[scriby]

Do you think you might be able to publish these changes to npm? Thanks!

[Krinkle]

@scriby Yep. I was going to do the build stuff as well so that it actually works as-is in a browser, but I suppose we can do that later and I don't have the time for it right now.

I'll cut a release as soon as #64 lands.

[Krinkle]

@scriby This is now done!

[scriby]

Thank you!