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

use path.join to connect file path #249

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bolasblack
Copy link

Then we can write ../styles/app.css in caches.main

@NekR
Copy link
Owner

NekR commented May 3, 2017

Hi! We'll need to test this one or probably even write some tests for it. Can you verify it doesn't break any tests right now? Also, what's the rationally behind using such paths?

@bolasblack
Copy link
Author

Hi, I have executed npm test and there are no test failed.

In my case, I only use webpack to bundle javascript, other assets (for example css) was generated by gulp. The folder structure is like this:

.
├── assets
├── gulpfile.js
├── public
│   ├── index.html
│   ├── scripts
│   ├── styles
├── scripts
├── styles
└── webpack.config.js

My webpack.config.js write like this:

module.exports = {
  output: {
    path: sysPath.resolve('./public/scripts'),
    publicPath: '/scripts',
    filename: '[name].js',
  },
  plugins: [
    new OfflinePlugin({
      externals: [
        'index.html',
        'styles/app.css',
      ].concat(glob.sync(
        '**/*',
        { cwd: './assets/', nodir: true }
      )).map(a => '../' + a),
      ServiceWorker: {},
      AppCache: {},
    }),
  ],
}

With current code, the final public/scripts/appcache/manifest.appcache will include some path like this:

/scripts/../
/scripts/../styles/app.css

So I think concat path should use path.resolve instead of +

@bolasblack bolasblack changed the title use path.resolve to connect file path use path.join to connect file path May 5, 2017
@NekR
Copy link
Owner

NekR commented May 5, 2017

@bolasblack Okay, I see the reasons. But do really do changes and see if tests fail or not you need to build the project first. See CONTIBUTING.md

@GGAlanSmithee
Copy link
Collaborator

@NekR seems like all CI's are passing. Would you like to merge this? Othervise, maybe we can close it :)

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.

3 participants