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

Check to see if bundle has been loaded already. Fixes #273 #275

Merged
merged 2 commits into from
Oct 16, 2017

Conversation

robframpton
Copy link

No description provided.

@jbalsas jbalsas added this to the 2.14.0 milestone Oct 11, 2017
@jbalsas
Copy link
Contributor

jbalsas commented Oct 11, 2017

Hey @Robert-Frampton, this looks good in general, I just have one question. Even though I tweaked the metal-soy-bundle build, I quite never remember how it is setup and what is what 😂.

What is the difference between metal-soy-bundle/build/bundle.js and metal-soy-bundle/lib/bundle.js? Shouldn't we try to load lib twice instead of lib/bundle.js and build/bundle.js?

While we're at this... could we add some information in metal-soy-bundle/README.md clarifying for future reference?

@robframpton
Copy link
Author

Hey @jbalsas

build/bundle.js is just the concatenated files, nothing more. That file is then transpiled which generates lib/bundle.js.

The reason I loaded the build/bundle.js file in the test, is that Karma has some kind of mechanism for making sure a file is loaded only once, so I couldn't get it to load a second time when pointing to lib/bundle.js.

And yes, we can add some readme info on the build process :)

@jbalsas
Copy link
Contributor

jbalsas commented Oct 11, 2017

I see... and couldn’t we trick Karma with some kind of cache querystring? &t=123?

@robframpton
Copy link
Author

That's what I tried actually, didn't have any affect on it.

@robframpton
Copy link
Author

Added a short description of the build process.

@jbalsas jbalsas merged commit 39ee3da into metal:develop Oct 16, 2017
@jbalsas
Copy link
Contributor

jbalsas commented Oct 16, 2017

Hey @Robert-Frampton, looks like this actually broke some tests when merged 😢. I'm trying to see why the merge tests are not running in Travis (only the PR ones).

In the meantime, I can revert this so we can process the others, or you can send a followup PR fixing it. What would you prefer? 🤷‍♂️

@yuchi
Copy link
Contributor

yuchi commented Oct 16, 2017

Question: does the solution to the issue at hand make impossible to have multiple Metal.js versions in the same page? Hint: it’s going to happen when deploying modern bundles to portal built with different metal version.

jbalsas added a commit that referenced this pull request Oct 18, 2017
@jbalsas
Copy link
Contributor

jbalsas commented Oct 18, 2017

Hey @yuchi, that's actually a very interesting question indeed... I don't think it has ever been possible, because both incr-dom and soy-bundle leak to the global scope.

We're going to revert it in any case. We can rather approach this with that in mind. It would involve touching both incr-dom and soy-bundle, so we'll have to plan that for some later time...

robframpton pushed a commit that referenced this pull request Oct 18, 2017
Revert "Merge pull request #275 from Robert-Frampton/metal-soy-bundle"
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