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

Avoid dependency on global document. #15

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Gozala
Copy link

@Gozala Gozala commented Oct 28, 2014

I would like to use this library in the context where there is no single global or document (It is a browser code that has access to every document loaded, but there is not a single one that we could pick) there for it isn't possible to provide something like require("global/document").

As far as I see createElement without document, is just for a convenience of the API, if it's really crucial maybe some other package could wrap this createElement to do that.

@neonstalwart
Copy link
Collaborator

do you have a problem if require("global/document") is still there and you provide your document via opts.document? with your change, it's now mandatory to provide opts.document but if we leave the dependency and it still works for you when you provide opts.document then i don't see a need to merge this change - we would still have convenience and flexibility whereas we would lose convenience by merging this.

@Gozala
Copy link
Author

Gozala commented Oct 28, 2014

@neonstalwart as I have pointed out I can not add require("global/document") in that environment, there for loading modules would just fail.

Also what's the problem with making different package that exposes convenient API, that way users desiring convenience could use it & users who can't use it will use more lower level API. I'm also fine with doing it other way round, meaning factoring out all of this into separate package say vdom-core and then exposing all of it here, but by wrapping createElement to preserve desired convenience.

@neonstalwart
Copy link
Collaborator

does require("global/document") cause an error in your environment? that's the part i'm still not clear on. i realize that it might give you something that's not useful but if it doesn't cause an error then what is the reason to change it?

@Gozala
Copy link
Author

Gozala commented Oct 28, 2014

does require("global/document") cause an error in your environment?

Such module does not exists in the environment I'm trying to use this, there for loading it will cause an error. I also won't be able to get a green light on landing fake require("global/document") just to use this library.

@neonstalwart
Copy link
Collaborator

I also won't be able to get a green light on landing fake require("global/document") just to use this library.

i see - that sounds like the real showstopper for you.

@Matt-Esch
Copy link
Owner

This still isn't making sense to me, let me try again to understand your problem.

Currently, you can always specify the document that you wish to use, correct? createElement takes an optional document. This was done on purpose to support the iframe use case.

We have global/document currently required which will default you to the available global document. If a global document is not available, it defaults to a lightweight emulation (min-document). By requiring min document we have a very efficient way to default the document to a sensible default.

I would like to use this library in the context where there is no single global or document

I think you currently can do this, as it was designed for the ifame/node case. I still don't understand why global/document cannot be used in your environment. As long as you provide the document you want to use, this doesn't get in the way.

if it's really crucial maybe some other package could wrap this createElement to do that.

I do think it is important to set the defaults.

Such module does not exists in the environment I'm trying to use this, there for loading it will cause an error.

Can we fix global/document to not error in your environment? I think it's important that is doesn't anyway. If you're saying the global/document module doesn't exist, I fail to see why there is no issue with vdom and vtree not existing either.

@Gozala
Copy link
Author

Gozala commented Oct 28, 2014

Can we fix global/document to not error in your environment? I think it's important that is doesn't anyway. If you're saying the global/document module doesn't exist, I fail to see why there is no issue with vdom and vtree not existing either.

Ok so inclusion of vdom + vtree to our project is already a hard sell that involves convincing team members. Now selling this with all the extra burden (like dependencies that don't really make any logical sense to our environment) is likely impossible, because both memory footprint and overall product size are constraints for the project. Given present constraints my attempt to use vdom + vtree will either be rejected or at best I'll have on option to make fork that strips out all the extra burden. Neither of these options is ideal & I would much rather have shared subset that everyone could collaborate on.

@Raynos
Copy link
Collaborator

Raynos commented Oct 28, 2014

@Gozala I can fix the global module to not load min-document in the browser.

The fact it does that is a bug.

In an ideal world when you browserify require('global/document') you get back module.exports = document

@Gozala
Copy link
Author

Gozala commented Oct 28, 2014

In an ideal world when you browserify require('global/document') you get back module.exports = document

Well we don't actually use browserify. I don't know what do you consider fix to be, but the fact that we would have to add global package to our project is kind of what makes it harder sell.

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

Successfully merging this pull request may close these issues.

5 participants