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

Refactored packages into multiple packages #10

Merged
merged 3 commits into from
Mar 10, 2016
Merged

Conversation

mitar
Copy link
Contributor

@mitar mitar commented Mar 3, 2016

Followup of: meteor/meteor#5903

@mitar
Copy link
Contributor Author

mitar commented Mar 3, 2016

This is work in progress. The issue is that it is really hard to test any of this now that it is outside of the Meteor repository and it is unclear how versions work.

@mitar
Copy link
Contributor Author

mitar commented Mar 3, 2016

This does not really work. One issue is that dynamic.html from templating-runtime package cannot be compiled, because there is no plugin to process HTML at that moment.

But the whole point of this refactoring is to make so that I can reuse what is currently in templating-runtime and replace only the HTML compilation plugin with something else.

@stubailo, @zol, so if we are testing collaboration here, here is the moment where some guidance from MDG would be useful, to see how to resolve this, so that I can continue working on the pull request. So what would be the solution for this.

@zol
Copy link

zol commented Mar 4, 2016

Thanks for your work on this Mitar. @martijnwalraven - I'd like to get your thoughts on this and Mitar's question on #5903

@martijnwalraven
Copy link
Contributor

@mitar: I can't really help you with the specifics of what you're trying to do, because I don't know much about Blaze. But I would like to find a way for you to test the changes you're making and pave the way to including these in future Meteor releases.

On meteor/meteor#5903, you ask when Blaze will be removed from themeteor/meteor repository. My thinking was that we want to get an RC of 1.3 out first (should be really soon now), and then remove the split off packages from devel. The easiest short term solution to use the packages in meteor/blaze is probably to symlink them from a meteor/meteor checkout. Do you think that would work for you?

We're not sure yet what the best way is to do a release with split off packages. Depending on when we want to do the next release, it may make more sense to find a way to publish these packages on NPM, which is where we want all core packages to end up eventually. In the meantime, we could use symlinks for the release process or modify the publish-release code to include external packages.

@zol mentioned you are also interested in finding a way to publish Blaze as an NPM package. Do you have any idea yet how you would like to approach this?

@mitar
Copy link
Contributor Author

mitar commented Mar 5, 2016

Hm, I think we have some parallel conversations here. No problem. Let me see if I can add anything useful to them.

I can't really help you with the specifics of what you're trying to do, because I don't know much about Blaze.

Oh, OK. The issue is about build plugins. So the question is to split current package for compiling HTML files into multiple packages so that the rest of the code could be reused, while plugin could be replaced with a different one.

Alternatively, there could be a way to change an existing plugin, or monkey-patching it (that would be enough for my particular situation), but from the prior discussion I think there is opposition to such idea:

So current approach is trying to split the package, but this approach is stuck because of current build system not supporting the idea that you add some HTML files in one package, and tell in another package how to compile those HTML files. (So where to put dynamic.html file.)

Personally, I would be for short and ugly solution would be access to build plugins. But I am willing to help make another approach, only it seems really hard. So I am asking for some guidance how to proceed with splitting those packages and making it work: body and dynamic template logic in one package, while plugin in another.

This was the main thing I wanted to discuss in this pull request. The rest we can also discuss, but it is probably for some other place:

My thinking was that we want to get an RC of 1.3 out first (should be really soon now)

You should make sure you merge this fix for bad pull request merge first: meteor/meteor#6392 (not sure if the previous pull request is part of the RC 1.3 though, if it is just in devel, then no uregncy).

The easiest short term solution to use the packages in meteor/blaze is probably to symlink them from a meteor/meteor checkout. Do you think that would work for you?

Symlinks have issues on Windows, no?

Maybe git submodules? But the problem is that there are multiple packages in this repository.

But the question is also how to test this repository on its own. This is the main idea behind decoupling packages. That they can also be independently tested.

modify the publish-release code to include external packages.

That would be in general useful. That one could use forked core packages as well.

you are also interested in finding a way to publish Blaze as an NPM package. Do you have any idea yet how you would like to approach this?

Oh, I do not have much experience with NPM. I mean, I do have some packages published, but I have been mostly inside Meteor ecosystem.

What would be interesting is to see how to port build plugins to NPM. If this is possible, maybe then this pull request is unnecessary and we should first convert Blase to NPM, and during that process split plugin into another NPM package.

But, that would also take a lot of time, I suspect. So what can we do in short term?

@martijnwalraven
Copy link
Contributor

Ok, let's focus on the pull request first and continue the conversation about working with split-off repositories somewhere else.

As I mentioned, I'm not actually that familiar with Blaze internals, so bear with me while I try to understand what you're trying to accomplish.

My first naive though is: wouldn't it be possible to always compile dynamic.html with the standard HTML compiler plugin? So to have templating-runtime depend on templating-plugin (would templating-compiler be a better name?). As far as I can see, that would only affect the way dynamic.html is compiled and other packages could still use an alternative compiler.

@mitar
Copy link
Contributor Author

mitar commented Mar 6, 2016

Hmm. You are maybe right. So we have high-level package templating which serves for backwards compatible purposes and implies templating-runtime which depends on templating-plugin, but does not imply it. So depending on the templating-runtime does not give you templating-compiler. This is why templating would imply templating-compiler as well.

Do you agree with this?

@martijnwalraven
Copy link
Contributor

Yes, that's what I had in mind. I'm glad you think that makes sense!

@mitar
Copy link
Contributor Author

mitar commented Mar 6, 2016

Fun fact: templating package had tests for dynamic templates, but has not really used them.

@mitar
Copy link
Contributor Author

mitar commented Mar 6, 2016

I moved things around and added a plugin as a dependency, but all is theoretical because I am unable to run tests.

@mitar
Copy link
Contributor Author

mitar commented Mar 9, 2016

Yes! It is running tests.

@martijnwalraven
Copy link
Contributor

But I'm seeing an error because meteor list needs to run in an app directory.

@mitar mitar closed this Mar 9, 2016
@mitar mitar reopened this Mar 9, 2016
@mitar
Copy link
Contributor Author

mitar commented Mar 9, 2016

Running again. But it does not look like anything is happening. :-(

@martijnwalraven
Copy link
Contributor

I don't think split-templating contains a circle.yml, so you may want to rebase on master.

@martijnwalraven
Copy link
Contributor

But then then you will get the meteor list error that I mentioned before, so you should remove that from the script first.

@mitar mitar closed this Mar 10, 2016
@mitar mitar reopened this Mar 10, 2016
mitar added a commit that referenced this pull request Mar 10, 2016
Refactored packages into multiple packages.
@mitar mitar merged commit 20b4db1 into master Mar 10, 2016
@mitar mitar deleted the split-templating branch March 10, 2016 02:00
@mitar
Copy link
Contributor Author

mitar commented Mar 10, 2016

Interesting. This broke things in a way that our CirlceCI testing did not fail because tests failed to even load.

The issue is that there is code around which does Package.templating.Template. And:

Package.onUse(function (api) {
  api.imply('templating-compiler');
  api.imply('templating-runtime');
});

Does not seem to be completely backwards compatible in this regard. Any trick here to do?

@mitar
Copy link
Contributor Author

mitar commented Mar 10, 2016

Fixed with: f2e61cd

@stubailo
Copy link
Contributor

Yes, I think that's the right fix.

@mitar
Copy link
Contributor Author

mitar commented Mar 10, 2016

But I would say that this is something imply should do automatically, no? Propagate exported symbols?

@stubailo
Copy link
Contributor

I dunno - I think in NPM/import world which we are heading to, imply won't even exist. So it won't be unexpected anymore - the set of things that a specific package exports will be clearly part of the API.

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.

4 participants