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

Use globally accessible (rather than local) source paths in JS source maps (#781) #783

Merged
merged 1 commit into from Nov 25, 2018

Conversation

Projects
None yet
2 participants
@djneades
Copy link
Contributor

commented Nov 23, 2018

@alexandru Please see what you think of this. I have not previously encountered the sbt-git plugin that the Monix build is using. I am also not sure of your versioning policies and preferences, but I have attempted to do something reasonably sensible along the lines of the Cats project as a first attempt at a PR.

I have tested this change (by running sbt package for the monix project) in three scenarios:

  1. When the HEAD of the local build repository is tagged with a version string. In this case, source maps reference appropriate tagged content on GitHub. e.g. https://raw.githubusercontent.com/monix/monix/v3.0.0-RC2/...

  2. When there are uncommitted changes in the local build repository (in which case, isSnapshot appears to be true). In this scenario, source maps reference the SHA hash of the most recent commit. i.e. https://raw.githubusercontent.com/monix/monix/<sha-hash>/...

  3. When there are committed but untagged changes. In this case, isSnapshot appears to be false, and the source maps reference the most recent tag that sbt-git was able to find.

Scenario (1) is probably all that most Monix users will care about, with (2) and (3) being of relevance to Monix developers.

It seems to me that the behaviour in scenario (3) is not ideal – it would probably be better to map to GitHub paths containing the SHA hash of the current commit. However, it is not clear to me whether it is the isSnapshot logic that is incorrect here – perhaps isSnapshot should be true in this instance? This will depend upon your definition of snapshot for the Monix project.

Finally, the behaviour of scenario (2) is also debatable. Maybe local paths should be preserved in this situation? Again, though, I’m unsure whether isSnapshot is acting correctly here – are we really building a snapshot if we are building from uncommitted changes? Given my lack of familiarity with sbt-git and your project policies, I am not sure how best to detect this situation.

Thus, I suspect this PR will need further work, and that it would make more sense for a regular Monix contributor to run with that given the policy questions involved. Still, perhaps this PR will be somewhat helpful as a starting point.

@alexandru

This comment has been minimized.

Copy link
Member

commented Nov 23, 2018

@djneades thanks a lot.

Btw, Cats-Effect probably needs the same treatment 🙂

@djneades

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2018

@alexandru You’re welcome. And yes, it probably does :-)

@alexandru alexandru merged commit 5f044c3 into monix:master Nov 25, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
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.