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

Bower to npm #8147

Merged
merged 15 commits into from
Dec 5, 2017
Merged

Bower to npm #8147

merged 15 commits into from
Dec 5, 2017

Conversation

nschonni
Copy link
Member

@nschonni nschonni commented Sep 15, 2017

Related to #8063 but there are a few left over that will take some more effort.

They don't have official NPM packages:

  • Flot
  • excanvas
  • SideBySideImproved

There is a fork/commit being used:

  • magnific-popup
  • openlayers

This could land as-is, but probably needs a good review to see it I overlooked anything when repointing stuff.

There is also a few dependies that are just dropped, since they were orphans from the v4 alpha stage

@LaurentGoderre
Copy link
Member

@nschonni can you also remove the lock file. Since we fix the node issue we shouldn't pin everything back like that

@nschonni
Copy link
Member Author

@LaurentGoderre the NPM 5 lock file is a little different in that it actually works 😉
I could remove it, but since we have a bunch of pinned dependencies, I think it makes sense to leave it until everything can move to a ^ dependency

@LaurentGoderre
Copy link
Member

Ok!

@duboisp
Copy link
Member

duboisp commented Oct 12, 2017

@nschonni can you fix the Conflicting file

Thanks

Gruntfile.coffee Outdated
@@ -1063,7 +1082,7 @@ module.exports = (grunt) ->
flatten: true
,
cwd: "lib/MathJax"
src: [
src: [
Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, need to fix whitespace in this file again since I think the MathJax added that accidentally

Since we can’t have multiple versions for 1.x and 2.x in the
package.json, the legacy 1.x is handled by downloading it into the old
location with wget.
Fixed the server message template not using the correct version

mmend
Let it bump up since the only difference was compatibility for module
loading
No compatible NPM published version, and it has no references in the
codebase
This is bundled with Mondernizr for < IE9 and isn’t referenced
Was previously unreferenced and is replaced with the HTML5shiv in
Modernizr
Left pinned to the old/existing version
This also moves it to the Official fork under the Google GH org instead
of an independent mirror
Kept it pinned at the old/outdated version
Left pinned at the existing version
@nschonni
Copy link
Member Author

nschonni commented Dec 1, 2017

This has been updated to include the MathJax changes and Magnific

@lucas-hay
Copy link
Contributor

@nschonni is this ready to merge now?

@nschonni
Copy link
Member Author

nschonni commented Dec 1, 2017

Yes

@lucas-hay
Copy link
Contributor

@LaurentGoderre are you OK with this being merged?

"selectivizr": "1.0.2",
"SideBySideImproved": "https://github.com/pkoltermann/SideBySideImproved.git",
"unorm": "^1.4.1"
"SideBySideImproved": "https://github.com/pkoltermann/SideBySideImproved.git"
Copy link
Member

Choose a reason for hiding this comment

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

Why can't these two go in the package.json?

Copy link
Member Author

Choose a reason for hiding this comment

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

NPM can only use a Git source if the repo has a package.json in the repo. Those currently don't

Copy link
Member

Choose a reason for hiding this comment

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

OOh!

Copy link
Member

Choose a reason for hiding this comment

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

Well openlayers we kind of control so you can create a PR there for a package.json

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, there is the separate PR to try and update OpenLayers, so I didn't want to cause more conflicts. I think newer version of the library are just published normally to NPM

Copy link
Member

Choose a reason for hiding this comment

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

OpenLayers is coming via NPM now in the latest PR.

@@ -22,27 +22,12 @@
"lib"
],
"dependencies": {
"bootstrap-sass-official": "3.3.1",
"DataTables": "1.10.13",
"es5-shim": "2.3.0",
"excanvas": "*",
Copy link
Member

Choose a reason for hiding this comment

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

We can fork this one and add a package.json. We wont need this in the future of WET so we're just keeping it to not breaking existing things.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, but that can be done then updated in a separate PR

Copy link
Member

Choose a reason for hiding this comment

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

I agree!

@nschonni
Copy link
Member Author

nschonni commented Dec 5, 2017

@LaurentGoderre do you want to land this?

@duboisp
Copy link
Member

duboisp commented Dec 5, 2017

@lucas-hay see @nschonni comments

@LaurentGoderre
Copy link
Member

I don't have time to test irt but if you're confident, I don't see why not!

@LaurentGoderre
Copy link
Member

@nschonni do you want a squash? I think we should merge multiple commits for this one.

@nschonni
Copy link
Member Author

nschonni commented Dec 5, 2017

No, I rebased it so each of the commits is pretty atomic, so I don't think the squash is needed

@LaurentGoderre LaurentGoderre merged commit ac343d8 into wet-boew:master Dec 5, 2017
@nschonni nschonni deleted the bower-to-npm branch December 5, 2017 20:31
@duboisp
Copy link
Member

duboisp commented Dec 5, 2017

@lucas-hay I think this is the PR that are currently breaking the GCWeb build.

@nschonni @LaurentGoderre can this PR be reverted for now and be merge at the next PR review meeting?

@lucas-hay
Copy link
Contributor

@nschonni @LaurentGoderre Can this be fixed in GCWEB or reverted here?
I will finally be releasing 4.0.27 tomorrow and my schedule can't afford any delays tomorrow. I would assume that perhaps the other repo's may have the same problem.

@duboisp duboisp mentioned this pull request Dec 5, 2017
@EricDunsworth
Copy link
Member

EricDunsworth commented Dec 5, 2017

@lucas-hay @duboisp I haven't tested this myself, but wet-boew/GCWeb#1320's Travis-CI error can probably be resolved by porting one of this PR's Gruntfile changes into GCWeb's Gruntfile.

I could send over a GCWeb PR with that change if needed. But I don't have time to test it locally and thus can't guarantee that there won't be any further errors past that point in the build process.

@lucas-hay
Copy link
Contributor

@EricDunsworth We would likely need to do this with all repos and with the release tomorrow I think the best bet is to revert for now and we can merge after release is complete and then check other repos.

@EricDunsworth
Copy link
Member

@duboisp @lucas-hay Are there any plans to re-implement this PR's changes (since #8228 reverted it)?

I personally would've preferred quick-fixing the other themes rather than reverting this PR. The changes it brought in were valid and didn't break this repo's build system.

IMO it would've been more ideal to fix the other themes' build systems - even if it resulted in 4.0.27 getting further delayed. Plus in the end, due to other issues, 4.0.27 didn't end up getting released until over a week after #8228 was merged in.

@duboisp
Copy link
Member

duboisp commented Jan 9, 2018

@EricDunsworth The first thing will be to have all those PR's ready to be merge. Then, after testing, we would be able to merge it and re-apply the change from this PR.

Feel free to submit all those related PR's then it can be merged all at once during a "PR review" meeting.

@LaurentGoderre
Copy link
Member

Testing this can't be done online btw. You would have to run a local npm link

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.

None yet

6 participants