-
Notifications
You must be signed in to change notification settings - Fork 237
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
Remove jQuery from default installation #1478
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this locally using the step by step (v1) example, I found that as expected with this code checked out the step by step page didn't work, but after running npm install jquery
it did. Very nice!
As a side note I also tested this commit with the changes to use the step by step extension, to verify that the scoped jQuery and packaged jQuery did not conflict. I found no issues.
Once the unit tests are fixed I think this is ready to merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded fileSystemPath needs to be calculated with path.resolve.
lib/extensions/extensions.test.js
Outdated
}) | ||
it('should not include public URLs for jQuery if it is not installed', () => { | ||
expect(extensions.getPublicUrlAndFileSystemPaths('assets')).toEqual([{ | ||
fileSystemPath: '/Users/natalie.carey/projects/opensource/alphagov/govuk-prototype-kit/node_modules/jquery/dist', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the hard coded fileSystemPath should be: path.resolve('node_modules', 'jquery', 'dist')
The test should be:
it('should not include public URLs for jQuery if it is not installed', () => {
expect(extensions.getPublicUrlAndFileSystemPaths('assets')).toEqual([{
fileSystemPath: path.resolve('node_modules', 'jquery', 'dist'),
publicUrl: '/extension-assets/jquery/dist'
}])
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, yeah, I think it shows that I was writing this at the end of the day :)
6d03301
to
4721f91
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue with step by step because jquery isn't there
@@ -1,6 +1,4 @@ | |||
<!-- Javascript --> | |||
<script src="/public/javascripts/jquery-1.11.3.js"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will users get this change as part of the update process?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No
73d29a9
to
002f524
Compare
This was missed from PR #1478.
This removes jQuery. If the user misses it they can install the latest version using
npm install jquery
, if they have problems with the latest version they can continue using the same version by usingnpm install jquery@1.11.3
. They can also choose their own version.Extensions may rely on jQuery under the cover but shouldn't expose it to the user, the user should be in control of their own jQuery version but not get one by default.
Closes #1420.
Closes #1448.