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

Remove jQuery from default installation #1478

Merged
merged 2 commits into from
Jul 22, 2022
Merged

Remove jQuery from default installation #1478

merged 2 commits into from
Jul 22, 2022

Conversation

nataliecarey
Copy link
Contributor

@nataliecarey nataliecarey commented Jul 21, 2022

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 using npm 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.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-remove-jqu-ytdznq July 21, 2022 16:17 Inactive
@lfdebrux lfdebrux self-requested a review July 21, 2022 16:22
Copy link
Member

@lfdebrux lfdebrux left a 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.

Copy link
Contributor

@BenSurgisonGDS BenSurgisonGDS left a 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.

})
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',
Copy link
Contributor

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'
        }])
      })

Copy link
Contributor Author

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 :)

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-remove-jqu-ytdznq July 22, 2022 10:36 Inactive
Copy link
Contributor

@BenSurgisonGDS BenSurgisonGDS left a comment

Choose a reason for hiding this comment

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

Looks good

@BenSurgisonGDS BenSurgisonGDS self-requested a review July 22, 2022 11:28
Copy link
Contributor

@BenSurgisonGDS BenSurgisonGDS left a 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>
Copy link
Contributor

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?

Copy link
Member

Choose a reason for hiding this comment

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

No

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-remove-jqu-ytdznq July 22, 2022 14:15 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-remove-jqu-ytdznq July 22, 2022 14:27 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-remove-jqu-ytdznq July 22, 2022 14:41 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-remove-jqu-ytdznq July 22, 2022 14:52 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-remove-jqu-ytdznq July 22, 2022 14:53 Inactive
@BenSurgisonGDS BenSurgisonGDS merged commit a5b4adb into main Jul 22, 2022
@BenSurgisonGDS BenSurgisonGDS deleted the remove-jquery branch July 22, 2022 15:14
@lfdebrux lfdebrux restored the remove-jquery branch July 22, 2022 15:16
@lfdebrux lfdebrux deleted the remove-jquery branch July 22, 2022 15:16
@lfdebrux lfdebrux changed the title Removing jQuery and allowing the user to install it if they miss it. Remove jQuery from default installation Oct 31, 2022
lfdebrux added a commit that referenced this pull request Oct 31, 2022
@lfdebrux lfdebrux mentioned this pull request Nov 17, 2022
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.

Remove vendored jQuery from kit Support jquery as an extension
5 participants