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

Move external dependencies to node_modules #6028

Merged
merged 4 commits into from
Jan 29, 2018

Conversation

cherniavskii
Copy link
Collaborator

This PR removes hardcoded versions of expect and sinon and adds them to devDependencies

@ghybs
Copy link
Collaborator

ghybs commented Jan 29, 2018

Hi,

I like the cleanup! 👍

For history purpose, expect and sinon were initially hard-coded as was done for jasmine, in order to easily launch the spec/index.html test file in a browser without npm install prerequisite.
See PR #1479

However, right from the evolution of that PR, other libs were included, using npm dependencies.
Therefore there is absolutely no reason keeping these 2 libs hard-coded instead of using them through dependencies (i.e. through node_modules).

These hard-coded versions were:

  • expect: 0.1.2, replaced by ^0.3.1 in this PR (through karma-expect version ^1.1.3) (current latest 1.20.2)
  • sinon: 1.6.0, replaced by ^1.6.0 in this PR (current latest 4.2.2).

Copy link
Collaborator

@ghybs ghybs left a comment

Choose a reason for hiding this comment

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

LGTM, tests pass

@ghybs ghybs merged commit c3a6a5b into master Jan 29, 2018
@ghybs
Copy link
Collaborator

ghybs commented Jan 29, 2018

BTW we should also mention that the leaflet-include.js file / symlink is no longer relevant since the shift to Rollup build: browsers no longer understand modules with import and export (except through new <script type="module"> syntax, but support is still narrow).

Therefore we now need to build the dist file in order to be able to launch tests from the spec/index.html page.
Hence replacing leaflet-include.js by dist/leaflet-src.js directly is absolutely appropriate.

@cherniavskii cherniavskii deleted the move-externa-dependencies-to-node_modules branch January 29, 2018 09:29
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.

2 participants