cssinjs / css-vendor

Runtime vendor prefixing based on feature detection.
MIT License
67 stars 24 forks source link

Function API #2

Closed jasonkuhrt closed 9 years ago

jasonkuhrt commented 9 years ago

The current implementation automatically executes upon require. It makes it hard to write isomorphic JavaScript. I am trying to deliver a feature https://github.com/littlebits/react-popover/issues/34 that is blocked by my use of css-vendor:

/Users/jasonkuhrt/projects/react-popover/node_modules/css-vendor/lib/prefix.js:16
var style = document.createElement('p').style
            ^
ReferenceError: document is not defined
    at Object.<anonymous> (/Users/jasonkuhrt/projects/react-popover/node_modules/css-vendor/lib/prefix.js:16:13)
    at Module._compile (module.js:460:26)
    at Module._extensions..js (module.js:478:10)
    at Object.require.extensions.(anonymous function) [as .js] (/usr/local/lib/node_modules/babel/node_modules/babel-core/lib/api/register/node.js:214:7)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Module.require (module.js:365:17)
    at require (module.js:384:17)
    at Object.<anonymous> (/Users/jasonkuhrt/projects/react-popover/node_modules/css-vendor/index.js:9:18)
    at Module._compile (module.js:460:26)

If we give consumers control over the execution then they can react at runtime and hence make their program isomorphic. Would you accept a PR that changes the API of this module as described?

kof commented 9 years ago

why do you try to use css-vendor on the server?

kof commented 9 years ago

you do realize that this package only makes sense in the browser?

jasonkuhrt commented 9 years ago

@kof I believe if you review the linked use-cases it should be clear. Writing Isomorphic JavaScript requires that the code adapts at runtime. Perhaps a future approach could see Isomorphic JavaScript resolved statically but that is way out of scope whereas this requested feature trivial: Give the user a function instead of executing code on their behalf.

jasonkuhrt commented 9 years ago

The use-case is Server Side Rendering. In this case the API I'm working with is React.renderToString but there is simply nothing here particular to React. The principal of Server Side Rendering and Isomorphic JavaScript is generic.

kof commented 9 years ago

So you never have if (weAreOnServer) do this ... you expect EVERY single module to adapt if we are on server even if a module was designed to run in browser???

kof commented 9 years ago

Does it works for things like localStorage etc?

kof commented 9 years ago

ohhhhhh I get now the prob .... its because require is called before you can actually decide whether you need some module or not in a particular env, got it.

kof commented 9 years ago

give me a sec

jasonkuhrt commented 9 years ago

: D

kof commented 9 years ago

fixed and published, thanks

jasonkuhrt commented 9 years ago

Are you sure you published?

npm ERR! notarget No compatible version found: css-vendor@'>=0.2.4 <0.3.0'
npm ERR! notarget Valid install targets:
npm ERR! notarget ["0.1.0","0.2.0","0.2.1","0.2.2","0.2.3"]

Note missing 0.2.4 that you said you published.

kof commented 9 years ago

apparantly not ... sorry ... but now

jasonkuhrt commented 9 years ago

Also some gaps in your initial patch (sorry to be the bearer of lame news):

jasonkuhrt commented 9 years ago

apparantly not ... sorry ... but now

Yep works now thanks! Just a few parts of the codebase that need to be updated now. Want me to get a PR going for this to help or not?

kof commented 9 years ago

sure, prs are always welcome ...

jasonkuhrt commented 9 years ago

Ok great. I'll try to put something together in the near future. Also I suggest we re-open this issue until said PR is merged etc.

kof commented 9 years ago

oh so its still the same issue with dom?

jasonkuhrt commented 9 years ago

For this issue to be resolved all dependencies on the Browser need to be guarded by environment checks which is what you have started to do. The alternative solution I suggested was to store these dependencies inside functions. The links above show where this library still makes unqualified use of the browser environment.

kof commented 9 years ago

oh right, I checked only the one use pointed to first.

kof commented 9 years ago

ok fixed and published again.

jasonkuhrt commented 9 years ago

Still not working:

/Users/jasonkuhrt/projects/react-popover/node_modules/css-vendor/lib/supported-value.js:27
    el.style[property] = value
      ^
TypeError: Cannot read property 'style' of undefined
    at Object.module.exports (/Users/jasonkuhrt/projects/react-popover/node_modules/css-vendor/lib/supported-value.js:27:7)
    at cssvalue (/Users/jasonkuhrt/projects/react-popover/lib/index.js:18:9)
    at Object.<anonymous> (/Users/jasonkuhrt/projects/react-popover/lib/index.js:25:12)
    at Module._compile (module.js:460:26)
    at normalLoader (/usr/local/lib/node_modules/babel/node_modules/babel-core/lib/api/register/node.js:199:5)
    at Object.require.extensions.(anonymous function) [as .js] (/usr/local/lib/node_modules/babel/node_modules/babel-core/lib/api/register/node.js:216:7)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Module.require (module.js:365:17)
    at require (module.js:384:17)

But I may take a different strategy so don't worry about it... Thanks.

kof commented 9 years ago

well this is because you actually use this plugin. What I was fixing is that you can't handle per environment setup because it throws before. Now you can, you just don't use the plugin serverside.

kof commented 9 years ago

means if (!NODE_ENV) jss.use(vendorPrefixer)

jasonkuhrt commented 9 years ago

@kof Pardon me I tested against the wrong code. Looking again it appears to be working thanks.