Skip to content
This repository has been archived by the owner on Jul 11, 2019. It is now read-only.

Add leaflet.pm #14

Merged
merged 8 commits into from
Nov 8, 2018
Merged

Add leaflet.pm #14

merged 8 commits into from
Nov 8, 2018

Conversation

mathieuleclaire
Copy link
Contributor

This PR contains several things:

  • the leaflet.pm facade
  • a refactoring of the example according to
  • the use of direct js dependencies from npm using https://github.com/ISCPIF/scalajs-execnpm. It makes simpler (than jsDependencies) to add js dependencies to a project seamless.

@mathieuleclaire
Copy link
Contributor Author

Hey, any remark on this PR ? I would be interested in using this in prod. Thanks !

addSbtPlugin("org.scalastyle" %% "scalastyle-sbt-plugin" % "1.0.0")
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this getting added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was still there. I just upgraded the version from 0.8 to 1.0 in my previous PR.

@hibikir
Copy link
Contributor

hibikir commented Nov 5, 2018

The dependency with leafletpm is absolutely welcome.
I wonder about adding your own plugin, which you are the only contributor to. So we really need it?

@mathieuleclaire
Copy link
Contributor Author

mathieuleclaire commented Nov 5, 2018

I developed this to take advantages of the npm mechanism. Actually it is also implemented in scalajs-bundler but it comes in my plugin with less stuff. What I wanted is getting a mechanism that permits to depends on js libraries (as we would depend on a scala library) and to gather all js deps in one single deps.js, with no extra stuff like webpack. The js deps are managed transitively. And it works fine; it is just a couple of sbt rules to make the scala-js dev easier. And you are very welcome to contribute in this plugin scalajs-execnpm
, so that I am not the only dev any more ;)

@mathieuleclaire
Copy link
Contributor Author

So ?

@hibikir hibikir merged commit 2a80f41 into cibotech:master Nov 8, 2018
@mathieuleclaire
Copy link
Contributor Author

Thanks !

@mathieuleclaire
Copy link
Contributor Author

Could you publish a version ? A snapshot is enough for now if you prefer not to release now.

jfklingler pushed a commit that referenced this pull request Nov 8, 2018
These changes broke the release process.

This reverts commit 2a80f41, reversing
changes made to 213bfce.
@jfklingler
Copy link
Contributor

The changes to build.sbt broke the release process. There are several changes here that don't make much sense: the root project was removed, several settings were duplicated across modules, the example project is missing settings required by the release plugin, etc.

@jfklingler jfklingler mentioned this pull request Nov 8, 2018
@mathieuleclaire
Copy link
Contributor Author

OK, I will fix this. Do you really think the example needs to be published ?

@jfklingler
Copy link
Contributor

Well, I don't think it needs to be published. I don't think it was being published before.

@mathieuleclaire
Copy link
Contributor Author

Well, I don't see any problem in the build.sbt. All the modules depends on the globalSettings (except the example one). I can't test it, I don't have credentials (and it is the only error I get when I try to publish). What error do you get if you publish ?

@jfklingler
Copy link
Contributor

As I mentioned, the root project was removed long with its config. I'm not sure why that was necessary. So, for example, the original bintrayOrganization was

[info] leaflet-facade/*:bintrayOrganization
[info] 	Some(cibotech)
[info] leaflet-draw/*:bintrayOrganization
[info] 	Some(cibotech)
[info] root/*:bintrayOrganization
[info] 	None

But after the changes, it's now

[info] example/*:bintrayOrganization
[info] 	None
[info] leaflet-pm/*:bintrayOrganization
[info] 	Some(cibotech)
[info] leaflet-draw/*:bintrayOrganization
[info] 	Some(cibotech)
[info] leaflet-facade/*:bintrayOrganization
[info] 	Some(cibotech)
[info] default-274479/*:bintrayOrganization
[info] 	None

The two errors I see are in regards to the default-274479 module:

  • you must define at least one license for this project
  • was not able to find or create a package for jfklingler in Repo(jfklingler,maven) named default-274479 - This is my personal bintray repo when it should be cibotech

@mathieuleclaire
Copy link
Contributor Author

It's normal you don't get a None for example since the settings are not set for this module. I removed the root so that each module is published separately. It is cool to only depends on one aspect and not on the whole project. Typically, someone could be interested in the leaflet-facade but not in the pm one.

So, the error is about the default-274479, which should be a kind of root project. It is created because the 'homepage' and the 'headerLicence' variables are defined outside the globalSettings. I do not remember why I set them outside. Moving them in the globalSettings should fix the problem.

@jfklingler
Copy link
Contributor

So, if people won't want to depend on the pm module, why is it there? What's its purpose? I do understand that default-274479 is sbt creating a module that it needs but wasn't named. What I don't understand is why the root module was removed.

@mathieuleclaire
Copy link
Contributor Author

Someone may be interested in leaflet-facade only but not in pm. The modularity enables to choose exactly what you want and not to depend on modules you don't need. Imagine, we add 100 other modules around leaflet, it is sure that a no one will need all of them together. In this context, the root project is useless. Try a publishLocal, you'll see the result. Did you try to move the homepage and the headerLicence in the globalSettings ?

mathieuleclaire added a commit to mathieuleclaire/leaflet-facade that referenced this pull request Nov 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants