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

FLUID-5458 (for the 1.5.x branch) #540

Open
wants to merge 3 commits into
base: infusion-1.5.x
Choose a base branch
from

Conversation

jobara
Copy link
Member

@jobara jobara commented Jul 3, 2014

Updated the .npmignore file and updated the version numbers to 1.5.1 in preparation for the 1.5.1 tag.

http://issues.fluidproject.org/browse/FLUID-5458

products
project.xml
project.properties
pom.xml
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove project.xml, project.properties, and pom.xml now that we've removed Ant and Maven from our build process?

@colinbdclark
Copy link
Member

After reviewing and testing this pull request, I started to think through what we want to include in our Node module. There are two things that are currently left in, which we may not want to include:

  • The ProgressiveEnhancement library
  • The Preferences Framework

Both of these are DOM-dependent. The Preferences Framework is likely also dependent on jQuery UI, which we omit. Should we exclude these two libraries from our Node module as well, then?

Thoughts?

@jobara
Copy link
Member Author

jobara commented Jul 11, 2014

@colinbdclark is the issue with the progressiveEnhancement around the use of "window"? I think that progressiveEnhancement could generally be useful. Perhaps we just need to be more careful with the use of or inclusion of the built in feature detection functions. Maybe we could just fix up the issues for 1.5.1 and 2.0? What do you think?

Removed old excludes that related to ant files that no longer exist. Also added the preferences framework to be ignored.
@colinbdclark
Copy link
Member

Hi @jobara, yes the issue is that there are a number of DOM-dependent parts to the current ProgressiveEnhancement.js file. It's not so much the use of "window," as the fact that we've got several checks--supportsBinaryXHR and supportsFormData in particular--that are expecting to be run in a browser environment, as well as the section at the end of the file that toggles CSS classes.

We would need to factor these checks out into separate files and think clearly about how they get linked to the environment. My impression is that this might be a bit much for the 1.5.1 release, but I'm cool if you think it's doable. It certainly would be useful to have this feature available in for Node.js code.

Alternatively, we could exclude the feature in our Node module until 2.0. What do you think makes the most sense?

@jobara
Copy link
Member Author

jobara commented Jul 11, 2014

@colinbdclark, supportsBinaryXHR and supportsFormData don't run automatically. I suppose we could factor them out into a separate file, but I don't think they should be too harmful. The css stuff only runs if we are in a browser, so this should also be safe, although again maybe it could be factored into a different file. What do you think would be best?

@colinbdclark
Copy link
Member

Okay, that's fine. It seems like we should factor out the browser-specific code for 2.0, even if it doesn't run by default. We really should have some basic unit tests to show that our Node module actually works, since in the past we have regressed because of issues similar to this.

@jobara
Copy link
Member Author

jobara commented Jul 11, 2014

I've filed a couple new jiras related to the issues mentioned above.:

I've also filed another pull request to add some of the resent changes into master. #544

This pull request is ready for review again.

@jobara jobara changed the title FLUID-5458 FLUID-5458 (for the 1.5.x branch) Jul 17, 2014
@amb26
Copy link
Member

amb26 commented Aug 11, 2014

Hi @jobara - your previous comment includes the same JIRA twice - could you double-check the number? I placed a comment on #544 suggesting that removing the prefs framework from npm may be premature, similar to the status of progressiveEnhancement. I think we need to work harder to factor DOM/non-DOM code apart from each other before we can achieve this.

In the day when we have the "new renderer", this distinction will become even more unclear, since even viewComponents will be capable of running on the server.

@jobara
Copy link
Member Author

jobara commented Aug 13, 2014

@amb26 Thanks for catching that. I've updated the comment with the other jira number.

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.

3 participants