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

feat(plugins): allow to import separate plugins #26767

Merged
merged 1 commit into from
Jul 19, 2018

Conversation

Johann-S
Copy link
Member

@Johann-S Johann-S commented Jun 27, 2018

Finally I succeed to achieve that !

With that PR, folks will be able to choose which plugins they want to import and not all of our plugins.

Examples:

Importing Util

import $ from 'jquery';
import Util from 'bootstrap/js/dist/util';

Importing Popover

import $ from 'jquery';
import 'popper.js';
import 'bootstrap/js/dist/tooltip';
import 'bootstrap/js/dist/popover';

This PR would help a lot in that case: symfony/webpack-encore#338

TODO

  • Update our docs

Fixes: #26757

}

// Do not bundle Tooltip in Popover
if (bsPlugins[pluginKey] === bsPlugins.Popover) {
Copy link
Contributor

Choose a reason for hiding this comment

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

woudln't pluginKey === 'Popover' be simpler ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep it'll clearer thanks @stof 👍

@@ -0,0 +1,80 @@
/*!
* Script to build our plugins to use them separately.
* Copyright 2017-2018 The Bootstrap Authors
Copy link
Member

Choose a reason for hiding this comment

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

nitpicking, but this file is new, so 2018 is enough.

@XhmikosR
Copy link
Member

XhmikosR commented Jul 2, 2018

  1. Isn't there any way to not add yet another set of generated files to the repo?
  2. Shouldn't we test these files too?

@Johann-S
Copy link
Member Author

Johann-S commented Jul 2, 2018

Yep I'm not confortable with adding a new set of dist files... I thought about deleting our js/dist folder but it can be helpful for those who don't use bundlers

Those files don't have to be tested because the logic is the same.

Bundlers are used a lot nowaday, so we should be able to provide distinct plugins

@XhmikosR
Copy link
Member

XhmikosR commented Jul 2, 2018

Please wait for @mdo's feedback too.

@mdo
Copy link
Member

mdo commented Jul 3, 2018

This wouldn't be a patch release, this would be a minor or major as it adds new functionality IMO. Could be convinced otherwise though.

That aside, does anything change once we've removed the jQuery dependency? Does this gain us anything long term? Is there a path forward without generating more dist files?

@Johann-S
Copy link
Member Author

Johann-S commented Jul 3, 2018

Nope nothing will change about that when jQuery will be removed, I choose to build our plugins in UMD (our other dist files are in UMD too, except js/dist) because it's compatible with most of bundlers so we'll keep those file for a long time.

Unfortunately there is no other path, for example Popper.js (see https://cdnjs.com/libraries/popper.js) is built in three format (UMD, ESM and IIFE)

@stof
Copy link
Contributor

stof commented Jul 3, 2018

The issue is that even without jQuery, your plugins still depend on Util, and these internal dependencies are incompatible with bundlers if they rely on global variables all the time.

One solution could be to turn the existing js/dist into UMD ones all the time. It would still work for people using them globally (that's the point of UMD: it works for both global-based loading and CommonJS)

@Johann-S
Copy link
Member Author

Johann-S commented Jul 3, 2018

Unfortunately it's doesn't work if we build our js/dist files in UMD, because the scope of Util won't be reachable for our other plugins, that's why I let them in IIFE

@stof
Copy link
Contributor

stof commented Jul 3, 2018

@Johann-S that's not true. UMD builds can still expose their export to be consumed by others. That's the whole point of UMD.

@Johann-S
Copy link
Member Author

Johann-S commented Jul 3, 2018

I tried what you said @stof and that's what I think too, but that's not what I get... Maybe I miss something in my Rollup config 🤔

@Johann-S
Copy link
Member Author

Johann-S commented Jul 9, 2018

Finally I found a solution using only Babel without Rollup.

But for those who do not use a bundler, to use our plugins they will have to add ['default'] for example:

Util['default'].getUid('test')

For me it's a breaking change, but with this solution no new dist files... Any feedbacks ?

BTW we can suggest in our documentation a quick hack to avoid this :

Alert = Alert['default']
Util = Util['default']

/CC @XhmikosR and @mdo

@mdo
Copy link
Member

mdo commented Jul 12, 2018

@Johann-S I don't know enough about it at the moment to make a call myself. Might need to ask you to explain more over Slack?

@Johann-S
Copy link
Member Author

Sure @mdo will talk about it on Slack 👌

@Johann-S
Copy link
Member Author

Hi everybody !

Finally I made a change in my script, and...

  • No new dist files
  • No breaking change
  • One dev deps nuked

So IMO, we should merge that asap.

/CC @XhmikosR if you can check my docs change 😄 , and @mdo to notice you

@Johann-S Johann-S merged commit eb81c39 into v4-dev Jul 19, 2018
@Johann-S Johann-S deleted the v4-dev-jo-bundle-plugins branch July 19, 2018 17:59
@mdo mdo mentioned this pull request Jul 19, 2018
@donni106
Copy link

@Johann-S how does jquery work for you? i always get TypeError: $ is undefined (referenced in util.js:64:5)

import $ from 'jquery';
import 'bootstrap/js/dist/util';
import 'bootstrap/js/dist/carousel';

my project is setup with a rollup config where i already tried a thousand things for making jquery work :(

@Johann-S
Copy link
Member Author

Pretty difficult to help you without seeing your Rollup config @donni106

@donni106
Copy link

thanks. ok, currently it is the following. but i tried with setting globals or commonjs stuff before. i read a few things but nothing made any changes.

const path = require('path');
const babel = require('rollup-plugin-babel');
const resolve = require('rollup-plugin-node-resolve');
const commonjs = require('rollup-plugin-commonjs');

const fileDest = 'script.js';

module.exports = {
  input: path.resolve(__dirname, '../src/tx_theme/js/main.js'),
  output: {
    banner: '',
    file: path.resolve(
      __dirname,
      `../dist/typo3/typo3conf/ext/theme/Resources/Public/JS/${fileDest}`
    ),
    format: 'umd',
    globals: {},
    name: 'script',
  },
  external: ['jquery'],
  plugins: [
    babel({
      exclude: 'node_modules/**',
    }),
    resolve(),
    commonjs(),
  ],
};

@donni106
Copy link

Oh damn it seems to work with removing 'jquery' from external.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants