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

Discussion: Version 3 release #97

Closed
3 of 4 tasks
bholloway opened this issue Aug 19, 2018 · 12 comments
Closed
3 of 4 tasks

Discussion: Version 3 release #97

bholloway opened this issue Aug 19, 2018 · 12 comments

Comments

@bholloway
Copy link
Owner

bholloway commented Aug 19, 2018

Finally rework has been updated to postcss!

An alpha is available as resolve-url-loader@next. This dist-tag will be updated as we near release.

Refer to the readme for migration instructions.

Breaking Changes

  • Multiple options changed or deprecated.
  • Removed file search "magic" in favour of join option.
  • Errors always fail and are no longer swallowed.
  • Processing absolute asset paths requires root option to be set.

Feedback required

Moving forward on this release is contingent to a few questions..

  • is your existing build unchanged when using engine: rework ?
  • is your build working when using engine: postcss (i.e. the new default) ?
  • do you need the file-search "magic" that was removed; is your use-case broken without it ?

And it would be nice to know...

  • does this fix any existing issues ?

Maturity

For an alpha this seems (to me) pretty robust.

The only API that is in question is the join function. This is advanced feature and is unlikely to be needed by most users. If you intend to use a custom join (in place of the file search "magic") then please let me know.

I've not tested any complex (real-world) builds. If you have a use-case you can encode into an example project then I'm definitely interested.

@n-rodriguez
Copy link

Hi there! I'm testing it on my app and it's already in production :)

Here's my setup :

  • webpack 4.17.1
const { environment } = require('@rails/webpacker')

// Load Webpack config
const bootstrap          = require('./loaders/bootstrap')
const datatables         = require('./loaders/datatables')
const erb                = require('./loaders/erb')
const jquery             = require('./loaders/jquery')
const modernizr_loader   = require('./loaders/modernizr_loader')
const modernizr_resolver = require('./loaders/modernizr_resolver')
const yadcf              = require('./loaders/yadcf')
const webpack            = require('webpack')

// Load resolve-url-loader
environment.loaders.get('sass').use.splice(-1, 0, {
  loader: 'resolve-url-loader'
})

// Replace existing coffee loader with CS2 version
const babel_loader = environment.loaders.get('babel')
environment.loaders.insert('coffee', {
  test: /\.coffee(\.erb)?$/,
  use:  babel_loader.use.concat(['coffee-loader'])
})

// Add Webpack loaders
environment.loaders.append('bootstrap', bootstrap)
environment.loaders.append('datatables', datatables)
environment.loaders.append('erb', erb)
environment.loaders.append('jquery', jquery)
environment.loaders.append('modernizr_loader', modernizr_loader)
environment.loaders.append('yadcf', yadcf)

// Merge Modernizr config
environment.config.merge(modernizr_resolver)

// Export configured environment
module.exports = environment

It solves an issue with JQueryUI where images were not exported.

Thank you!

@rjgotten
Copy link

rjgotten commented Aug 29, 2018

As requested in #98, some feedback on the two engines:

engine:rework seems to work without regression; but continues the trend of having path corruption in source map sources when building on Windows systems.

engine:postcss lessens the above path corruption considerably. (Only repeats the rooted disk path once, rather than n times.) However, the content of mapped sources is lost.

@n-rodriguez
Copy link

However, the content of mapped sources is lost.

The same here. See #98 (comment).

@bholloway
Copy link
Owner Author

bholloway commented Aug 30, 2018

So as discussed in #98 there is a fix for output source-map missing sourcesContent.

This is published as version 3.0.0-alpha.2 with updated dist-tag next.

@n-rodriguez
Copy link

This is published as version 3.0.0-alpha.2 with updated dist-tag next.

Awesome! It's working well :)

@rjgotten
Copy link

rjgotten commented Sep 5, 2018

Working well sofar for the project I'm working on as well.
Haven't heard of further issues than those resolved with alpha.2

@bholloway
Copy link
Owner Author

@rjgotten from my side I am checking correctness of a billion source-maps from the e2e tests. Following that I am happy to make a beta release.

Once at beta I am unsure. This is a big rewrite so I am wondering how cautious to be.

Either

  • First publish a patch of version 2 to npm that publicises the @next version in capitals.
  • Just push the button on release and hope for the best.

Happy to take thoughts. If there is any regressions with release then I might be having to make frantic fixes and imposing on those here to retest.

@bholloway
Copy link
Owner Author

bholloway commented Sep 7, 2018

On the subject of outgoing source-maps...

engine:postcss 👍

The mappings look good for the 10 cases in the e2e tests. I reviewed both development and production output.

The e2e tests are relatively simple cases but it gives some confidence, and really this is postcss concern anyhow.

engine:rework

The mappings look okay in development. 👍
They look okay in production for webpack < 4 👍
However they seem to be systematically flawed in production for webpack = 4 💩

@bholloway
Copy link
Owner Author

This is published as version 3.0.0-beta.1 with updated dist-tag next.

@bholloway bholloway mentioned this issue Sep 8, 2018
3 tasks
@bholloway
Copy link
Owner Author

bholloway commented Sep 8, 2018

3.0.0-beta.1

Please refer to the beta on npm. Install using dist-tag resolve-url-loader@next.

As per the readme, version 3 brings postcss parser.

Please try the beta and vote 👍 or 👎 for release based on whether it works for your use case.

If you vote 👎 please post detail any problems below.

@bholloway
Copy link
Owner Author

bholloway commented Sep 13, 2018

Only 3 thumbs in a week. But I am inclined to release (today?) anyhow.

Either way I think it is time to be on master per PR #101 .

@bholloway
Copy link
Owner Author

🎉 Published as [email protected] 🎉

Thanks for all your help!

I shall close the issue but please feel free to continue commenting.

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

No branches or pull requests

3 participants