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

Allow for register_engine without deprecation #345

Merged
merged 1 commit into from
Jul 21, 2016

Conversation

schneems
Copy link
Member

Frustratingly you may need to use both register_engine to get the behavior your want in Sprockets 3 even if you're using the new API (i.e. register_transformer etc.). I think this is because the different APIs have a different backend but I haven't looked into it enough to know exactly why they diverged.

Here's a PR that shows the problem: sass/sassc-rails#65

If you only call register_transformer then the Sprockets 3 tests will fail. I think this is because the processor built into lib/sprockets.rb and registered via register_engine still gets executed. By registering the sassc processor again with register_engine we're writing over the built in processor to ensure ours takes priority. I'm not 100% on this cause but it seems reasonable.

This PR introduces a way to silence the deprecation for register_engine so that libraries that still need the register_engine API to function correctly (such as sassc-rails) can operate without deprecations. This is an opt-in flag, we assume if you use the flag that you're aware of the new API and have updated your library appropriately.

After this PR I'm going to change the docs to show how to use both register_transformer and register_engine at the same time.

@rails-bot
Copy link

warning Warning warning

  • Pull requests are usually filed against the master branch for this repo, but this one is against 3.x-deprecations. Please double check that you specified the right target!

@rafaelfranca
Copy link
Member

Looks good to me.

This is not related to the pull request but why would you need sassc-rails with sprockets 4? sassc-rails is supposed to be included in sprockets 4.

@schneems
Copy link
Member Author

Sassc support is broken right now. Couldn't get it working on codetriage. Will have to work on a fix.

@schneems schneems force-pushed the schneems/allow-no-deprecations branch from 62e110e to 480acf7 Compare July 21, 2016 13:45
Frustratingly you may need to use both `register_engine` to get the behavior your want in Sprockets 3 even if you're using the new API (i.e. `register_transformer` etc.). I think this is because the different APIs have a different backend but I haven't looked into it enough to know exactly why they diverged.

Here's a PR that shows the problem: sass/sassc-rails#65

If you only call `register_transformer` then the Sprockets 3 tests will fail. I think this is because the processor built into `lib/sprockets.rb` and registered via `register_engine` still gets executed. By registering the sassc processor again with `register_engine` we're writing over the built in processor to ensure ours takes priority. I'm not 100% on this cause but it seems reasonable.

This PR introduces a way to silence the deprecation for `register_engine` so that libraries that still need the `register_engine` API to function correctly (such as sassc-rails) can operate without deprecations. This is an opt-in flag, we assume if you use the flag that you're aware of the new API and have updated your library appropriately.

After this PR I'm going to change the docs to show how to use both `register_transformer` and `register_engine` at the same time.
@schneems schneems force-pushed the schneems/allow-no-deprecations branch from 480acf7 to 0bafcdb Compare July 21, 2016 14:38
@schneems schneems merged commit 5a8edce into 3.x-deprecations Jul 21, 2016
@amatsuda
Copy link
Member

amatsuda commented Jul 21, 2016

@schneems Maybe I'm repeating the same question with @rails-bot's one above, but are you sure you wanted to put this warning in 3.7.0?

Since rails 5.0.0's default generated Gemfile bundles sass-rails '~> 5.0' which depends on sprockets '>= 2.8', '< 4.0', every newbie who performs rails new, even a Rails Girl who firstly touches Rails tomorrow in Tokyo, sees this DEPRECATION WARNING message on their console now. 🙈 🙈 🙈

@rafaelfranca
Copy link
Member

It is being fixed in sass-rails.

@amatsuda
Copy link
Member

I see. Thanks! ❤️

@schneems
Copy link
Member Author

The fixes are taking longer than expected due to the 5-0-stable having existing failing tests that are taking a long time to fix.

Also didn't see that sprockets 2.x register_engine has a different method signature :(

def register_engine(ext, klass)
  ext = Sprockets::Utils.normalize_extension(ext)
  @engines[ext] = klass
end

Not sure how to reconcile that second one.

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.

4 participants