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

Integrate PurgeCSS directly into Tailwind #1639

Merged
merged 4 commits into from
Apr 28, 2020
Merged

Integrate PurgeCSS directly into Tailwind #1639

merged 4 commits into from
Apr 28, 2020

Conversation

adamwathan
Copy link
Member

@adamwathan adamwathan commented Apr 28, 2020

This PR adds a new purge config option for purging unused CSS directly from within Tailwind instead of having to pull in and configure PurgeCSS manually.

The API is much more opinionated and slimmed down by default, optimizing for the 95% use case:

// tailwind.config.js
module.exports = {
  purge: [
    './src/**/*.html',
    './src/**/*.html',
  ],
  theme: {},
  variants: {},
  plugins: [],
}

The purge option accepts an array of filenames/globs to scan for classes, and that's it. This should be enough for most people, but if you are one of those people who just can't help themselves and has to fiddle with everything, you can fine-tune the configuration however you like (keep reading for more on that).

Things to note:

  • This option is only enabled when NODE_ENV is production. If you want to enable it explicitly based on some other conditional logic, you can use an object syntax and the enabled property, and specify your templates using the content property:

    // tailwind.config.js
    module.exports = {
      purge: {
        enabled: true,
        content: ['./src/**/*.html'],
      },
      // ...
    }
  • Only components and utilities are purged by default. Our base styles are not purged, and no custom CSS authored in your CSS files is purged, including any imports to outside libraries.

    This is to make sure you don't accidentally run into common pitfalls, like forgetting to scan your root HTML template (which is difficult with frameworks like Next.js since it is buried in node_modules) or accidentally purging styles that come from whatever datepicker library you've pulled in as an npm dependency.

    This means we are not purging absolutely everything that could be safely purged, but you get 99% of the benefit with much less manual tuning. You will basically never need to whitelist anything using this approach.

    Essentially we make your CSS file look like this:

    /* purgecss start ignore */
    @tailwind base;
    
    /* purgecss end ignore */
    @tailwind components;
    /* purgecss start ignore */
    
    .btn {
      @apply bg-blue-500 px-4 py-2 text-white;
    }
    
    .alert {
      @apply border p-4 rounded;
    }
    
    /* purgecss end ignore */
    @tailwind utilities;
    /* purgecss start ignore */
    
    .custom-utility {
      foo: bar;
    }
    /* purgecss end ignore */

    If you really want to purge everything, you can do so using mode: 'all':

    // tailwind.config.js
    module.exports = {
      purge: {
        mode: 'all',
        content: ['./src/**/*.html'],
      },
      // ...
    }

    You're much more likely to accidentally remove important styles if you do this but I'm not the cops — if it tickles your pickle go for it.

  • We are using a new extractor strategy that tries to be as permissive as possible.

    Here is the new extractor we're using:

    defaultExtractor: content => {
      // Capture as liberally as possible, including things like `h-(screen-1.5)`
      const broadMatches = content.match(/[^<>"'`\s]*[^<>"'`\s:]/g) || []
    
      // Capture classes within other delimiters like .block(class="w-1/2") in Pug
      const innerMatches = content.match(/[^<>"'`\s.()]*[^<>"'`\s.():]/g) || []
    
      return broadMatches.concat(innerMatches)
    }

    ...and here is the sample HTML we test our purging against:

    <!-- Basic HTML -->
    <div class="bg-red-500 md:bg-blue-300 w-1/2"></div>
    
    <!-- Vue dynamic classes -->
    <span :class="{ block: enabled, 'md:flow-root': !enabled }"></span>
    
    <!-- JSX with template strings -->
    <script>
      function Component() {
        return <div class={`h-screen`}></div>
      }
    </script>
    
    <!-- Custom classes with really weird characters -->
    <div class="min-h-(screen-4) bg-black! font-%#$@ w-(1/2+8)"></div>
    
    <!-- Pug -->
    span.inline-grid.grid-cols-3(class="px-1.5")
      .col-span-2
        Hello
      .col-span-1.text-center
        World!

    It works really well and I would be pretty surprised if you ever needed to customize this.

If you want more control than what we offer, you can pass any options you want directly to PurgeCSS using the options property:

// tailwind.config.js
module.exports = {
  purge: {
    enabled: true,
    content: ['./src/**/*.html'],

    // These options are passed through directly to PurgeCSS
    options: {
      whitelist: ['bg-red-500', 'px-4'],
    }
  },
  // ...
}

Any options passed to PurgeCSS will override our own options, so for example if you pass content it will override our content property at the top-level.

Happy purging 🤮

@MrJmpl3
Copy link

MrJmpl3 commented Apr 28, 2020

Please, don't forget add a example with prefix 😄

@utkarshkukreti
Copy link

Right now this won't work with Pug templates because we are allowing . characters in class names (Tailwind UI has classes like ml-0.5 for example). This is unfortunate but I can't think of a solution that allows both :/

How about matching the regex you currently have AND the same thing without dots?

> const c = 'foo bar.baz';
undefined
> c.match(/[^<>"'`\s]*[^<>"'`\s:]/g).concat(c.match(/[^<>"'`\s]*[^<>"'`\s:]/g))
[ 'foo', 'bar.baz', 'foo', 'bar.baz' ]
> c.match(/[^<>"'`\s]*[^<>"'`\s:]/g).concat(c.match(/[^<>"'`\s.]*[^<>"'`\s:.]/g))
[ 'foo', 'bar.baz', 'foo', 'bar', 'baz' ]

@h311x
Copy link

h311x commented Apr 28, 2020

JSX uses “className” not “class” :)
Also would it be capable of allowing us to use conditional class names in vue templates like this:

:class="{[someComputedValueThatReturnsAString]: true}"

@adamwathan
Copy link
Member Author

JSX uses “className” not “class” :)

Thankfully not important for this.

Also would it be capable of allowing us to use conditional class names in vue templates like this:
:class="{[someComputedValueThatReturnsAString]: true}"

Only if the computed value contains the entire class string and isn't dynamically calculated with weird string interpolation or anything. All of the regular limitations of PurgeCSS are unchanged here, read our guide on writing purgeable HTML:

https://tailwindcss.com/docs/controlling-file-size/#writing-purgeable-html

@adamwathan
Copy link
Member Author

How about matching the regex you currently have AND the same thing without dots?

@utkarshkukreti Good idea! Updated it to support Pug a bit better using your solution 👍

@brandonpittman
Copy link

@adamwathan Will this pass through whitelisting?

@kirkbushell
Copy link

haha this is great. I just added a little script on my forge deployments to do exactly this!

@klaasgeldof
Copy link

klaasgeldof commented Apr 28, 2020

Great improvement!

Unfortunately, the ignore annotations don't work when importing external css into sass (see FullHuman/purgecss#170). For example, I have this in app.scss one of my projects:

@tailwind base;

@tailwind components;

/* purgecss start ignore */
@import "~toastr/toastr.scss";
@import "~@fancyapps/fancybox/dist/jquery.fancybox.css";
/* purgecss end ignore */

@tailwind utilities;

But this still strips all of the css of toastr and fancybox. If you have a solution for this, that would be great 😃

Otherwise I guess it's still possible to pass whitelistPatterns?

@dominikzogg
Copy link

dominikzogg commented Apr 28, 2020

Using ReactJS would mean add *.jsx or *.tsx to the list?

Using VueJs would mean add *.vue to the list?

No other changes required?

@tbillington
Copy link

no custom CSS authored in your CSS files is purged

Could this possibly be configurable?

@OwenMelbz
Copy link
Contributor

This is awesome 🤩

What’s the chances of you allowing to pass in a whitelist to ignore? I feel like many people use the whitelist... especially on CMS driven websites where classes might come from the database 😇

@diwms
Copy link

diwms commented Apr 28, 2020

Anyone already trying this with Gatsby? Please, share your feedback if yes!

@berndartmueller
Copy link

Very nice feature! Thx!

I did a quick test with your regex pattern and pug syntax and it turns out that following pug code does not properly support purging css class names:

span.inline-grid.grid-cols-3(class="px-1.5")
  .col-span-2
    Hello
  .col-span-1.text-center
    World! 

Output of regex matching is following:
["span.inline-grid.grid-cols-3(class=", "px-1.5", ")", ".col-span-2", "Hello", ".col-span-1.text-center", "World!", "span", "inline-grid", "grid-cols-3(class=", "px-1", "5", ")", "col-span-2", "Hello", "col-span-1", "text-center", "World!"]

As you can see, grid-cols-3 is wrongly extracted.

I had this issue already a few weeks ago and I came up with the solution to do the class extraction in multiple passes (following is postcss-purgecss config):

defaultExtractor: content => {
    const firstPassMatches = content.match(/[\w-/:]+(?<!:)/g) || [];
    const secondPassMatches = content.match(/[\w-/.:]+(?<!:)/g) || [];
    const combined = firstPassMatches.concat(secondPassMatches);
    const unique = Array.from(new Set(combined));

    return unique;
  },

- Add better Pug support
- Add "modes", with "all" and "conservative" by default
- Allow passing options through to PurgeCSS
- Rename `paths` to `content` to match PurgeCSS
@adamwathan
Copy link
Member Author

But this still strips all of the css of toastr and fancybox. If you have a solution for this, that would be great

@klaasgeldof The solution in your case is to use the preservable comment syntax for Sass:

/** purgecss start ignore */

Note the double asterisk at the beginning 👍

Either way this issue won't impact this feature, these comments are added dynamically by Tailwind during the PostCSS phase, which is well after Sass is done looking at the files.

@adamwathan
Copy link
Member Author

Using ReactJS would mean add *.jsx or *.tsx to the list?

Using VueJs would mean add *.vue to the list?

No other changes required?

@dominikzogg Correct, you just pass an array of paths to where all of your templates are and you're done (as long as you follow the rules of writing purgeable HTML).

@dominikzogg
Copy link

@dominikzogg Correct, you just pass an array of paths to where all of your templates are and you're done (as long as you follow the rules of writing purgeable HTML).

@adamwathan perfect

@adamwathan
Copy link
Member Author

I've pushed an update that makes things more configurable. Details below!

Passing options directly to PurgeCSS

Now you can pass an options object which will be passed directly to PurgeCSS so you have access to any other PurgeCSS features you want like whitelisting, overriding the extractor, whatever you want.

// tailwind.config.js
module.exports = {
  purge: {
    content: ['./src/**/*.html'],

    // These options are passed directly to PurgeCSS
    options: {
      whitelist: ['bg-red-500', 'px-4']
    }
  },
  // ...
}

I've also renamed the paths option to content to match PurgeCSS.

Better Pug support

I've improved the matching strategy a bit more to better handle Pug. Works pretty great now, even on stuff like this:

span.inline-grid.grid-cols-3(class="px-1.5")
  .col-span-2
    Hello
  .col-span-1.text-center
    World!

New mode feature

As I mentioned in the original post, this PurgeCSS integration is designed to be fairly conservative by default and only purges styles generated by @tailwind components and @tailwind utilities to make it easier to integrate with third-party tools and to avoid accidentally stripping base styles when working with frameworks that hide their base HTML template in node_modules like Next.js.

If you want to purge everything (at your own risk of course), you can now do so by setting mode to all:

// tailwind.config.js
module.exports = {
  purge: {
    mode: 'all'
    content: ['./src/**/*.html'],
  },
  // ...
}

The default mode is "conservative" in case you're ever inclined to set it explicitly:

// tailwind.config.js
module.exports = {
  purge: {
    mode: 'conservative'
    content: ['./src/**/*.html'],
  },
  // ...
}

@hkhanna
Copy link

hkhanna commented Apr 28, 2020

This is huge. For folks (me) who accidentally purged things like the nprogress css styles and then are dumbfounded why the progress bar is not showing up, this will ease hours of frustration.

@Haqverdi
Copy link

is it will be enabled by default?

@adamwathan
Copy link
Member Author

is it will be enabled by default?

@Haqverdi Only if you have actually provided a list of paths and NODE_ENV is production. If you leave the purge option out of your config entirely it will never be enabled.

@josemarcilio
Copy link

This is awesome. Can't wait to use it!

@dominikzogg
Copy link

@adamwathan updated to 1.4 and adapted the config, works like a charm (react, vue)

chubbyts/react-petstore@5c27bcb
chubbyjs-legacy/petstore-vue@92fd622

@shirazz
Copy link

shirazz commented May 2, 2020

const purgecssConfig = {
  content: ["./components/**/*.jsx", "./pages/**/*.jsx"],
  whitelistPatterns: [/^PhoneInput/, /^Toastify/, /^alice-carousel/],
  whitelistPatternsChildren: [/^Toastify/],
  defaultExtractor: (content) => {
    const broadMatches = content.match(/[^<>"'`\s]*[^<>"'`\s:]/g) || [];
    const innerMatches = content.match(/[^<>"'`\s.()]*[^<>"'`\s.():]/g) || [];
    return broadMatches.concat(innerMatches);
  },
};

module.exports = {
  plugins: {
    "postcss-import": {},
    tailwindcss: {},
    "postcss-nested": {},
    "postcss-custom-properties": {},
    autoprefixer: {},
    "@fullhuman/postcss-purgecss":
      process.env.NODE_ENV === "production" ? purgecssConfig : false,
  },
};

In my NextJS app, I have a postcss.config.js file like the above. Updated the tailwind to new version which has the purgeCSS support inbuilt. I tried removing the postcss-purgecss plugin and added the purge css configuration in my tailwind.config.js Unfortunately it's not working. Can anyone please guide me here to make this work.

@adamwathan
Copy link
Member Author

Can you create a project that reproduces the issue so I can better help troubleshoot?

@shirazz
Copy link

shirazz commented May 3, 2020

@adamwathan Really sorry this was purely my fault. For some reason my tailwind was not updated to 1.4. Once i did it, everything worked as a charm. Thank you very much for adding this feature out of the box.

BTW. the only issue which i face is, while using a custom variant, tailwind-dir where my ltr: styles are purged. I haven't gone deep into this. but the issue is there. I can provide a repo if it helps.

@adamwathan adamwathan deleted the purgecss branch May 3, 2020 19:04
@divdax
Copy link

divdax commented May 4, 2020

Today i updated tailwind 1.4 to use purgecss. One thing i've noticed is that .mr-1.5 was purged. The class is in use in one of my blade files.

@klaasgeldof
Copy link

@divdax Had the same thing, this fixed it for me:

purge: {
  options: {
    whitelistPatterns: [/^.[^\.]*\.[^\. "']*/],
  },
},

@adamwathan Maybe include this pattern by default?

@divdax
Copy link

divdax commented May 4, 2020

It works! Thank you @klaasgeldof! 🙏 And yes... this pattern should be included by default.

@devcircus
Copy link

Not sure the framework should include white list patterns out of the box. That should probably be handled via an extractor pattern, like the one suggested by tailwindui.

@adamwathan
Copy link
Member Author

adamwathan commented May 4, 2020

@divdax That class should not be purged using our default settings, we even have a test for that here:

https://github.com/tailwindcss/tailwindcss/blob/master/__tests__/purgeUnusedStyles.test.js#L49

Can you create a project that reproduces the issue?

@klaasgeldof
Copy link

I was wrong. Before Tailwind had purge baked in, the classes with dots were purged and I had to add the pattern. I copied my patterns (including this one) to the Tailwind purge config because I supposed it would behave the same. But now I removed the pattern and it works... @divdax Check if there is no other purging going on in your production build apart from the Tailwind purge.

@agcty
Copy link

agcty commented May 9, 2020

@adamwathan What do you mean with:

  • Essentially we make your CSS file look like this:
    /* purgecss start ignore */
    @tailwind base;
    
    /* purgecss end ignore */
    @tailwind components;
    /* purgecss start ignore */
    
    .btn {
      @apply bg-blue-500 px-4 py-2 text-white;
    }
    
    .alert {
      @apply border p-4 rounded;
    }
    
    /* purgecss end ignore */
    @tailwind utilities;
    /* purgecss start ignore */
    
    .custom-utility {
      foo: bar;
    }
    /* purgecss end ignore */

We still need to add some sort of tailwind.scss file and import it globally so all our components can use tailwind classes right? Would we copy the contents of your comment? Or could we just use:

@tailwind base;
@tailwind components;
@tailwind utilities;

and the right stuff is purged automatically without having to use /* purgecss start ignore */ comments?

@adamwathan
Copy link
Member Author

You don't have to copy the CSS I pasted there, it's just explaining how it's working conceptually to people who are used to configuring PurgeCSS with the ignore comments manually.

@claudiodekker
Copy link

Super cool to see this as part of the core!

For those who want to combine this with Laravel (and possibly also for those migrating from spatie-mix-purgecss), here's the configuration set spatie-mix-purgecss was using, which might give you a good starting point:

module.exports = {
  purge: {
      // enabled: true,
      content: [
          "app/**/*.php",
          "resources/**/*.html",
          "resources/**/*.js",
          "resources/**/*.jsx",
          "resources/**/*.ts",
          "resources/**/*.tsx",
          "resources/**/*.php",
          "resources/**/*.vue",
          "resources/**/*.twig",
      ],
      options: {
          // defaultExtractor: (content) => content.match(/[\w-/.:]+(?<!:)/g) || [],
          whitelistPatterns: [/-active$/, /-enter$/, /-leave-to$/, /show$/],
      }
  },
  theme: {
    extend: {},
  },
  variants: {},
  plugins: [],
}

If you want 100% the same exact behavior as laravel-mix-purgecss, you can uncomment the defaultExtractor option. If you're not sure and just want something that works, just stick with this default, as it's pretty much tailor-fit to TailwindCSS.

Hope this helps!

@cygnusross
Copy link

Super cool to see this as part of the core!

For those who want to combine this with Laravel (and possibly also for those migrating from spatie-mix-purgecss), here's the configuration set spatie-mix-purgecss was using, which might give you a good starting point:

module.exports = {
  purge: {
      // enabled: true,
      content: [
          "app/**/*.php",
          "resources/**/*.html",
          "resources/**/*.js",
          "resources/**/*.jsx",
          "resources/**/*.ts",
          "resources/**/*.tsx",
          "resources/**/*.php",
          "resources/**/*.vue",
          "resources/**/*.twig",
      ],
      options: {
          // defaultExtractor: (content) => content.match(/[\w-/.:]+(?<!:)/g) || [],
          whitelistPatterns: [/-active$/, /-enter$/, /-leave-to$/, /show$/],
      }
  },
  theme: {
    extend: {},
  },
  variants: {},
  plugins: [],
}

If you want 100% the same exact behavior as laravel-mix-purgecss, you can uncomment the defaultExtractor option. If you're not sure and just want something that works, just stick with this default, as it's pretty much tailor-fit to TailwindCSS.

Hope this helps!

Very useful, thank you!

@cll0ud
Copy link

cll0ud commented May 24, 2020

I got it working fine on nuxt.js with Vuetify. I had to disable purgeCSS from nuxt.config.js because it was breaking Vuetify's style, after that it worked like a charm using the following config:

// tailwind.config.js
module.exports = {
  prefix: 'tw-',
  purge: {
    content: [
      './components/**/*.vue',
      './layouts/**/*.vue',
      './pages/**/*.vue'
    ],
    options: {
      whitelist: ['antialiased', 'font-sans']
    }
  },
  important: true,
  theme: {},
  variants: {},
  plugins: []
}

I noticed a little problem: I added the prefix "tw-" and when checking the generated css, I noticed that it had included ".tw-container" classes from Tailwind but I haven't used those at all. Thing is, Vuetify's got their own ".container" so it got ignored by the extractor even though I was using the prefix. Not a major problem but still unexpected result, I guess?

BTW, thanks for this feature, I love it!

@MalvinJay
Copy link

This saved me two nights of shame. 😢 👍

@silicahd
Copy link

silicahd commented Aug 9, 2020

This works great in a single app system. However in a CMS where you have lets say a User Front end and Admin section, how can you split the two css files.

As an example

purge: { enabled: true, content: [ './admin/**/Resources/views/*.blade.php', './users/**/Resources/views/*.blade.php' ], },

Is there a way to get the css for each page defined?

eg. user.css and admin.css

@OwenMelbz
Copy link
Contributor

Do you have 2 css files output from the same tailwind config?

Also, it's probably not recommended to use

purge.enabled = true

You should try use NODE_ENV as production and it's automatic then

@silicahd
Copy link

silicahd commented Aug 9, 2020

Do you have 2 css files output from the same tailwind config?

Also, it's probably not recommended to use

purge.enabled = true

You should try use NODE_ENV as production and it's automatic then

Thank you for the response, I think I figured out what I was looking to.

In order to create a Laravel mix file for backend and frontend each (and other entry points if needed).

For Laravel this is a must as it really makes not sense to have the css from frontend mixed with the backend. In reality what I will be working on is to make a css file for each page. This way you can take advantage of a super small css file in each page for use on something like AMP. I mean what is the point of using tailwindcss if we are not doing it all the way with Laravel :)

package.json

   "dev": "npm run development",
        "development": "cross-env NODE_ENV=development node_modules/webpack/bin/webpack.js --progress --hide-modules --config=node_modules/laravel-mix/setup/webpack.config.js",
        "watch": "npm run development -- --watch",
        "watch-poll": "npm run watch -- --watch-poll",
        "hot": "cross-env NODE_ENV=development node_modules/webpack-dev-server/bin/webpack-dev-server.js --inline --hot --disable-host-check --config=node_modules/laravel-mix/setup/webpack.config.js",
        "prod": "npm run production",
        "production": "cross-env process.env.section=website NODE_ENV=production node_modules/webpack/bin/webpack.js --no-progress --hide-modules --config=node_modules/laravel-mix/setup/webpack.config.js",
        "admin-dev": "npm run admin-development",
        "admin-development": "cross-env process.env.section=admin NODE_ENV=development node_modules/webpack/bin/webpack.js --progress --hide-modules --config=node_modules/laravel-mix/setup/webpack.config.js",
        "admin-watch": "cross-env process.env.section=admin NODE_ENV=development node_modules/webpack/bin/webpack.js --watch --progress --hide-modules --config=node_modules/laravel-mix/setup/webpack.config.js",
        "admin-prod": "npm run admin-production",
        "admin-production": "cross-env process.env.section=admin NODE_ENV=production node_modules/webpack/bin/webpack.js --progress --hide-modules --config=node_modules/laravel-mix/setup/webpack.config.js"

webpack.mix.js:

const mix = require('laravel-mix');
require('laravel-mix-tailwind');


if (mix.inProduction()) {
  mix
   .version();
}

if (process.env.section) {
   require(`${__dirname}/webpack.mix.${process.env.section}.js`);
 }

create webpack.mix.website.js:

const mix = require('laravel-mix');
require('laravel-mix-tailwind');


mix.js('resources/js/app.js', 'public/js')
   .postCss('resources/css/app.css', 'public/css')
   .tailwind('./tailwind.config.js');

create webpack.mix.admin.js:

const mix = require('laravel-mix');
require('laravel-mix-tailwind');


mix.js('resources/js/app.js', 'public/js')
.postCss('resources/css/app.css', 'public/css/admin')
.tailwind('./tailwind.admin.config.js')

create tailwind.admin.config.js:

module.exports = {
    purge: [
      './Modules/**/Resources/views/*.blade.php',
      './Modules/**/Resources/views/**/*.blade.php',
    ],
    theme: {
      extend: {}
    },
    variants: {},
    plugins: [
      require('@tailwindcss/custom-forms')
    ]
  }

@silicahd
Copy link

I have to point out, the above solution still does not fix the fundamental problem. As many know google will rate a website based on many aspects and one is Unused Css. So even if you run a purgecss and make the file smaller it will still have classes from other pages so you will still be penalized by google. In reality the solution would be to generate one css file per page each page on a website.

However it is still better than other frameworks.

@mklueh
Copy link

mklueh commented Oct 4, 2020

@virgiltu do you have a source for your statement?

@silicahd
Copy link

silicahd commented Oct 4, 2020

@virgiltu do you have a source for your statement?

Just use the google insight tool or lighthouse in dev tools. You will see google taking points for having unused CSS on the page. I am not sure how to add images but here is a list what you get penalized for.

Eliminate render-blocking resources
Remove unused CSS
Preload key requests
Defer offscreen images
Serve images in next-gen formats
Minify CSS
Remove unused JavaScript

@givebk-bot
Copy link

Hey! Looks like someone invited me to this issue.

@antl3x
Copy link

antl3x commented May 14, 2022

@givebk-bot !donate @adamwathan $1

thanks for this PR!

@givebk-bot
Copy link

givebk-bot commented May 14, 2022 via email

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.