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-6314: Fixing versioned infusion namespace. #922

Merged
merged 2 commits into from Aug 20, 2018

Conversation

Projects
None yet
3 participants
@jobara

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2018

@cindyli could you please review this PR?

var fluid_2_0_0 = fluid_2_0_0 || {};
var fluid = fluid || fluid_2_0_0;
var fluid_3_0_0 = fluid_3_0_0 || {};
var fluid = fluid || fluid_3_0_0;

This comment has been minimized.

Copy link
@cindyli

cindyli Aug 20, 2018

Member

This line seems unnecessary as fluid_3_0_0 is passed as an argument at line 992, which is accepted as a name fluid at line 18.

Looking thru the code base, currently there are 2 ways to scope a fluid version:

  1. Pre-define 2 global vars of fluid_3_0_0 and fluid as in this file;
  2. Only define a global var fluid_3_0_0 which is re-scoped by passing in as a function parameter when being used.

I wonder if we should take the chance to sync up the 2 usage? The option 2 looks simpler and more reasonable to me.

What do you think?

This comment has been minimized.

Copy link
@jobara

jobara Aug 20, 2018

Author Member

I believe the reason for var fluid = fluid || fluid_3_0_0 is if "fluid" itself isn't defined. In that case it becomes the fluid_3_0_0 object. I'm not sure it is necessary outside of fluid.js though.

This comment has been minimized.

Copy link
@cindyli

cindyli Aug 20, 2018

Member

Why does fluid need to be defined as it gets passed in as an argument with the value fluid_3_0_0?

This comment has been minimized.

Copy link
@amb26

amb26 Aug 20, 2018

Member

One definition is outside the closure, and the other one is inside.

Yes, historically there were two styles in order to reflect the different requirements of components which consumed the framework, but were versioned along with it (dubious in any case - since it should be possible to version components separately from the framework) such as InlineEdit, and on the other hand files like ModelTransformationTransforms.js which actually implemented the framework.

Things became a little further confused because for a while there were multiple profiles of the framework - e.g. "Fluid.js without FluidIoC.js" etc. I believe it is now the case that it is impossible to consume the framework without having first loaded Fluid.js and so we should be able to revert the more complex (two-line) boilerplate in every case except for that one.

@jobara

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2018

I've removed the var fluid = fluid || fluid_3_0_0 statement from files that require Fluid.js. There are however a few files that can be loaded independently: loadScripts.js, jquery.keyboard-a11y.js, and jquery.standalone.js. And, of course, Fluid.js still requires that statement.

@cindyli this PR is ready for more review.

@cindyli cindyli merged commit bd4a23f into fluid-project:master Aug 20, 2018

8 checks passed

buildkite/infusion Build #366 passed (38 minutes, 58 seconds)
Details
buildkite/infusion/browser-tests Passed (7 minutes, 52 seconds)
Details
buildkite/infusion/build Passed (7 minutes, 52 seconds)
Details
buildkite/infusion/cleanup Passed (11 seconds)
Details
buildkite/infusion/code-linter Passed (22 seconds)
Details
buildkite/infusion/node-tests Passed (1 minute, 3 seconds)
Details
buildkite/infusion/pipeline Passed (5 seconds)
Details
license/cla Contributor License Agreement is signed.
Details
@cindyli

This comment has been minimized.

Copy link
Member

commented Aug 20, 2018

Merged at db4f4fb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.