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

metal-soy-bundle shouldn't be so fragile #273

Closed
robframpton opened this issue Oct 9, 2017 · 4 comments
Closed

metal-soy-bundle shouldn't be so fragile #273

robframpton opened this issue Oct 9, 2017 · 4 comments

Comments

@robframpton
Copy link

robframpton commented Oct 9, 2017

In working on creating a lerna repo with multiple soy components in packages, I ran into the familiar error Uncaught Error: Namespace "goog.string" already declared..

In most scenarios we're already deduping our bundles which is fine, but it shouldn't be a requirement for simply importing more than one Soy component into a bundle, and it's not actually easy to perfectly dedupe in some testing environments. We should continue to dedupe, and encourage others to do the same, but metal-soy-bundle shouldn't be so fragile.

If it's a file that can truly only be loaded once in the client, then why not set some kind of flag to indicate that the bundle has been loaded already? Then check that flag before trying to invoke it a second time?

Here we are already prepending some logic to the file, if we add a simple snippet it should prevent those errors from being thrown.

if (this.__METAL_SOY_BUNDLE_LOADED__) {
  return;
}
this.__METAL_SOY_BUNDLE_LOADED__ = true;

We could even show a warning that the file has been imported multiple times, so that we're still encouraging deduping.

I suppose we could also check to see if goog.string is defined, I guess I just like the idea of being more explicit so the intention of the check is more obvious.

@jbalsas @brunobasto

Edit: just so it's more clear

@jbalsas
Copy link
Contributor

jbalsas commented Oct 10, 2017

Sounds good to me!

robframpton pushed a commit to robframpton/metal.js that referenced this issue Oct 11, 2017
@jbalsas jbalsas added this to the 2.14.0 milestone Oct 11, 2017
jbalsas added a commit that referenced this issue Oct 16, 2017
Check to see if bundle has been loaded already. Fixes #273
@jbalsas
Copy link
Contributor

jbalsas commented Oct 16, 2017

Merged into develop! 🎉

@jbalsas jbalsas closed this as completed Oct 16, 2017
@jbalsas
Copy link
Contributor

jbalsas commented Oct 18, 2017

We reverted this, so I'm reopening and removing the 2.14.0 milestone for now until we rethink this approach.

@yuchi, feel free to voice your concerns here!

@jbalsas jbalsas reopened this Oct 18, 2017
@jbalsas jbalsas removed this from the 2.14.0 milestone Oct 18, 2017
@yuchi
Copy link
Contributor

yuchi commented Oct 19, 2017

I do understand the libraries used behind the scenes are leaky, but Metal should work well even when different versions of it are loaded in the same page (as I said in the other issue).

I’ll have a look at the sources as soon as I can (that means in a while…).

@jbalsas jbalsas closed this as completed in bf01bee Nov 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants